-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Serialize Dns async-over-sync requests for the same host #49171
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #48566
|
1276438
to
60c1d50
Compare
@sebastienros I think we should do proxy test run here (httpclient and YARP). |
60c1d50
to
dedb940
Compare
Could the code here be simplified using a semaphore per hostname? |
That's basically what this is: "waiting on the semaphore" is done by waiting on the task currently holding the baton. If you mean actually having a SemaphoreSlim, for example, when is it removed from the dictionary to avoid a potential leak? |
Yeah, that's what I meant.
Ref count it under the lock. |
How does that look different from what I have here? At that point, you're retrieving a semaphore instead of a task from the dictionary, and waiting on semaphore.WaitAsync instead of task, but then also needing to call semaphore.Release()? |
It's probably not all that different; it mainly avoids using stuff like ContinueWith. For me, at least, I find it much easier to reason about simple awaits than to reason about ContinueWith, especially when you add stuff like TaskContinuationOptions.ExecuteSynchronously. I've gotten burned here in the past, so now my instinct is to try to avoid doing this if I can. But it's not a big deal either way. |
It doesn't really avoid it :) In order to be able to support the task getting canceled, you need to be able to do work after the task completes rather than as part of the task. That means either ContinueWith or a separate async method in which we await. I chose the former. If you would strongly prefer it, I can switch to await; I chose ContinueWith because the common case is that cancellation doesn't happen, and with ContinueWith, I can specify OnlyOnCanceled and avoid the continuation in the common case. |
Yeah, I understand. And this just makes me wish we optimized async methods better, so we wouldn't have to face that dilemma. But we're not going to fix that today :) I'm happy with the code as it is. |
The only person I accept using ContinueWith is @stephentoub. I've stopped using it myself though 😆 |
Fixes #48566
cc: @geoffkizer, @davidfowl