-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
Add Async method to SchemaBuilder to prevent possible thread starvation #508
Conversation
Unit tests are failing since adding Rollabck in the DbGenerator, only on Sql, maybe the connection is already disposing the transaction. We need to understand what is happening. I am not sure we should just remove it since it's working for other databases. But that's a safety net. |
Are you getting lazy, or just bored? |
I wish I was board and have nothing else better to do. |
@sebastienros I think all the issues are now fixed.
Now, everything support async which is great! Let me know if you see anything else or if you good with merging this one. |
@@ -630,7 +630,7 @@ public void Dispose(bool disposing) | |||
{ | |||
try | |||
{ | |||
CommitOrRollbackTransaction(); | |||
CommitOrRollbackTransactionAsync().ConfigureAwait(false).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't remember the details around this but are you sure about using ConfigureAwait(false)
before GetAwaiter()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it just handles the error differently.
Do you see an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would need more time to remember the details so not sure but reading again https://blog.stephencleary.com/2012/07/dont-block-on-async-code.html
For example this and that
Using ConfigureAwait(false) to avoid deadlocks is a dangerous practice.
The top-level methods, meanwhile, do require the context, so they cannot use ConfigureAwait(false)
I would not have used .ConfigureAwait(false)
here but inside CommitOrRollbackTransactionAsync()
, applied on the async call that it does.
Which may be useless if it is called asynchronously but maybe better for this synchronous usage.
But not sure ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the doubt I would not have used it at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I'll remove it tomorrow before releasing the new version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jtkech what about in the other places like Session.Save and the SchemeBuilder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idem for me.
Otherwise we may need to use it on each async call everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm, you suggest removing .ConfigureAwait(false) everywhere it's used? So .GetAwaiter().GetResult() only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix #509
Note, there was a bug in the dispose logic of the Session class. The method
CommitOrRollbackTransactionAsync
has slightly different logic thanCommitOrRollbackTransaction
. I believe the logic inCommitOrRollbackTransaction
is better and used.