Added an additional yield point prior to possibly long running initialization #115688
Conversation
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/ncl |
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs
Show resolved
Hide resolved
That's okay. This is already the case and something that It might still be worth adding the mitigation discussed in #115301 (comment) and similar, just in case we do hit the worst case of the init logic blocking for multiple seconds. |
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs
Show resolved
Hide resolved
Also added an explanation comment
between synchronous and asynchronous calls
This comment was marked as resolved.
This comment was marked as resolved.
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/tests/FunctionalTests/DiagnosticsTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Miha Zupan <[email protected]>
Co-authored-by: Miha Zupan <[email protected]>
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs
Show resolved
Hide resolved
to adhere to the code style
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| // The setup procedure is enqueued to thread pool to prevent the caller from blocking. | ||
| async Task<HttpResponseMessage> CreateHandlerAndSendAsync(HttpRequestMessage request, CancellationToken cancellationToken) | ||
| { | ||
| _handlerChainSetupTask ??= Task.Run(SetupHandlerChain); |
There was a problem hiding this comment.
If you want to optimize this a bit more and also remove the chance of double-setup you could use new Task(s => SetupHandlerChain(s), this) + CompareExchange + Task.Start (conditionally) and you don't need the method result here (so a void Task is sufficient), can use _handler after the setup task finishes.
There was a problem hiding this comment.
Sounds reasonable.
Already merged though. Do you think it justifies a separate issue? I'm not certain, because repeated initialization has been deemed not important.
There was a problem hiding this comment.
I don't think it's worth the extra logic.
…lization (#115688) * Added an additional yield point * Enqueued setup logic to thread pool Also added an explanation comment * Changed validation order to reduce discrepancies between synchronous and asynchronous calls * Updated comment Co-authored-by: Miha Zupan <[email protected]> * Removed Volatile.Read to make the code more readable Co-authored-by: Miha Zupan <[email protected]> * Removed the test that is no longer viable * Moved the nested method to adhere to the code style --------- Co-authored-by: Miha Zupan <[email protected]>
Closes #115301.
The changes are:
SendAsynccall and on the order of initialization and validation.Important note:
The setters of the class throw
InvalidOperationExceptionif the send operation is started on the instance of the class. This check is currently implemented based on whether inner_handlerisnullor not. Considering the current code assumes the possibility of multi-threaded initialization, this check is not super reliable.I've updated the test, which checks this behavior, to adjust to the new order of validation. If stronger guarantees should be provided, I can think about it, but it currently feels like everything (except for locking or
CompareExchange-ing additional flags around) would be only a best-effort attempt.