Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make transaction scope timeout the same as command timeout #612

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sebgod
Copy link

@sebgod sebgod commented Jan 16, 2025

No description provided.

@erikbra
Copy link
Owner

erikbra commented Jan 19, 2025

Hi, @sebgod ! Thanks for you PR. I'm not sure whether you meant to publish this as "I think this is ready to merge" or if it's just a draft? I see that this sets the timeout on the transaction. But do you have any particular use case/issue this fix solves? I don't know what the default timeout is here? Is it limited, or is it infinite? If it's infinite, I don't see a purpose in setting the timeout on both the transaction and the connection.

Could you please write a couple of word on which issue this PR solves? :)

@erikbra
Copy link
Owner

erikbra commented Jan 19, 2025

Will this one solve #572 , perhaps?

@sebgod
Copy link
Author

sebgod commented Jan 20, 2025

Will this one solve #572 , perhaps?

Hey @erikbra , yes that is the idea. I've yet to test this and well at a description

@erikbra
Copy link
Owner

erikbra commented Jan 20, 2025

Thanks. I'll wait until you have added a description too, then :) please reach out if you have any questions.

Please, also be aware of this property: TransactionManager.MaxTimeout. It seems to be set by default from Machine.config, I think it might be 10 minutes. So one might consider increasing that one too, if the timeout is set higher than 10 minutes in grate config.

https://learn.microsoft.com/en-us/dotnet/api/system.transactions.transactionmanager.maximumtimeout?view=net-9.0
https://github.com/imcarolwang/runtime/blob/5aa49453665fd8a7d8a9904cb81b463da648dba0/src/libraries/System.Transactions.Local/src/System/Transactions/Configuration/MachineSettingsSection.cs#L13

It might look likt the MaxTimeout is only set in .NET Framework? I'm not sure: https://learn.microsoft.com/en-us/dotnet/api/system.transactions.configuration.machinesettingssection.maxtimeout?view=netframework-4.8.1&viewFallbackFrom=net-9.0

@sebgod
Copy link
Author

sebgod commented Jan 20, 2025

Thanks. I'll wait until you have added a description too, then :) please reach out if you have any questions.

Please, also be aware of this property: TransactionManager.MaxTimeout. It seems to be set by default from Machine.config, I think it might be 10 minutes. So one might consider increasing that one too, if the timeout is set higher than 10 minutes in grate config.

https://learn.microsoft.com/en-us/dotnet/api/system.transactions.transactionmanager.maximumtimeout?view=net-9.0 https://github.com/imcarolwang/runtime/blob/5aa49453665fd8a7d8a9904cb81b463da648dba0/src/libraries/System.Transactions.Local/src/System/Transactions/Configuration/MachineSettingsSection.cs#L13

It might look likt the MaxTimeout is only set in .NET Framework? I'm not sure: https://learn.microsoft.com/en-us/dotnet/api/system.transactions.configuration.machinesettingssection.maxtimeout?view=netframework-4.8.1&viewFallbackFrom=net-9.0

Yeah I am also not quite sure how it works. We run grate in a container so relying on a machine config is troublesome at best, and a setting a specific value feels safer than relying on an unpredictable default behaviour.

I hope I can do some testing done today

@erikbra
Copy link
Owner

erikbra commented Jan 21, 2025

Yes, we should definitely set it explicitly, not rely on environmental settings. But it might be that we also need to increase the MaxTimeout property too, not only the timeout on the transaction itself, if the timeout should be configured to more than 10 minutes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants