-
Notifications
You must be signed in to change notification settings - Fork 151
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
Fix sync/async deadlock #404
Fix sync/async deadlock #404
Conversation
I don't think its a dependency we can take, but Microsoft's vs-threading repo has nice handlers for sync-over-async (aka JoinableTaskFactory) that perhaps would be a solution in this space. |
I don't know vs-threading, but looks like it wouldn't work for us indeed: https://github.com/microsoft/vs-threading/blob/master/src/Microsoft.VisualStudio.Threading/Microsoft.VisualStudio.Threading.csproj#L3, more specifically https://github.com/microsoft/vs-threading/search?q=RunContinuationsAsynchronously&unscoped_q=RunContinuationsAsynchronously (that's used by the JoinableTaskFactory). |
199b334
to
4306d1d
Compare
(cherry picked from commit 18730fa)
06b150a
to
ec0a317
Compare
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.
LGTM.
Thanks! |
throw; | ||
} | ||
|
||
return request.Waiter.Task.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.
Does this still timeout after timeout
milliseconds?
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, timeout
is passed through setupRequest
to InflightRequest
where it's used to set the timeout.https://github.com/nats-io/nats.net/blob/ec0a317bde5cf0b7d88a5e63c83ae43527d0274c/src/NATS.Client/Internals/InFlightRequest.cs#L72
Fixes a deadlock #395 using TaskCreationOptions.RunContinuationsAsynchronously on netstandard1.6.
No integration test because I couldn't make it fail reliably (before the fix).
Upgraded to net46 since #394 will do it anyways and it avoids an ugly workaround.