System.Net.Http.WinHttpResponseStream leading to crash in SafeWinHttpHandle.ReleaseHandle #24641
Comments
|
Sharing some interesting errors stacks in addition to already posted. The process crashed while the system was under .Net Core 2.0.0 in a state of high CPU load and lock contention .
Discussion
Thanks for looking into that. |
What is the purpose of this pattern? I.e. why have a shared static HttpClientHandler, but yet create a new HttpClient for every request? Why not just create a single HttpClient w/ a handler (HttpClientHandler) that can be shared static?
And was this same pattern being used when the application used .NET Framework 4.x? |
Both approaches (single HttpClient v.s. multiple HttpClients, single HttpMessageHandler) are argued at https://github.com/dotnet/corefx/issues/5811. Conclusion: HttpClient constrictor supports such pattern, so why not? Is there any reason to not use single HttpClientHandler - multiple HttpClients pattern? As of Dec 2017 there is known issue #18348 affecting singleton HttpClient. May also affect singleton HttpClientHandler, not sure about that. 2 days ago we updated the build to use static shared HttpClient on most (not every) active parts of the server application, reduced use of canceling of the requests in progress (infinite timeout, no cancellation tokens). 274 servers received the update. Result: 2 servers failed today on a quiet day within 24 hours after upgrade, in line with the usual rate of crashes. We are going to implement this pattern more consistently to see if the static HttpClient is making the system more reliable.
Yes, nothing changed. Thanks. |
This doesn't affect HttpClient, per se. It affects HttpClientHandler. HttpClient is a class that derives from HttpMessageInvoker. HttpMessageInvoker is a simple class that HttpClient itself adds some value over HttpMessageInvoker. It includes several convenience methods like PostAsync, GetAsync. It also has built-in ability to buffer the http response body (HttpCompletionOption enum choices). And it has some properties like HttpClient.Timeout that provide an easy way to set an overall timeout on the HTTP request. But HttpClient itself is not the HTTP stack. HttpClientHandler is considered the protocol stack. And from a design perspective, things like TCP connection pools, etc. are handled at the HttpClientHandler level. From a performance perspective, the only real thing of value in sharing objects is the handler. Recycling the HttpClient objects but keeping the HttpClientHandler static isn't really supposed to provide any added value than just creating a static HttpClient with an associated handler that is owned by the HttpClient. In fact, it probably just adds overhead in creating new HttpClient objects each time. So, that is why I asked this question because I don't understand the motivation for creating new HttpClient objects per request yet sharing the same static HttpClientHandler. |
There are convenience (HttpClient's individual properties) and historical (rooted in .net 4.0 WebRequest -based code) reasons to use a client instance per request. We realize it can be refactored to the singleton pattern. Putting aside the best practices: could the multiple client pattern contribute to the crashes in releasing of winhttp handles? If it could: we would accelerate the refactoring efforts. If not: what else shoudl we try as workaround? Thank you! |
I think it is possible that this particular pattern is related to the crashes. The pattern is not "illegal" per se, but it is not something that is tested extensively in the current test coverage. It is possible that this pattern is exposing a bug. So, as a workaround, it might be useful to see if changing the pattern helps with reducing these crashes. |
|
Looking at the HttpClient code, you can see that by recycling (disposing) the HttpClient object, you are essentially cancelling any pending requests that were started by the HttpClient: HttpClient itself tracks all requests that are pending so that it can support the So, my early assessment of this is that I think that this pattern is the "cause" of these crashes. |
|
@davidsh |
|
@davidsh @baal2000 David we can confirm that in the change made that was deployed over the weekend we stopped creating any static or shared HttpClientHandlers and we did create a static HttpClients and reused them. In the other places though (less frequently used), we use the HttpClient in a using statement, effectively closing the connection. These two and only ways we are using HttpClient and they are still seeing the very rare AV. |
|
@davidsh @stephentoub. Can you check this? This looks like a race condition WinHttpResponseStream.cs protected override void Dispose(bool disposing)
{
if (!_disposed)
{
_disposed = true;
if (disposing)
{
if (_requestHandle != null)
{
_requestHandle.Dispose();
_requestHandle = null;
}
}
}
base.Dispose(disposing);
}
private void CheckDisposed()
{
if (_disposed)
{
throw new ObjectDisposedException(this.GetType().FullName);
}
}
// The only way to abort pending async operations in WinHTTP is to close the request handle.
// This causes WinHTTP to cancel any pending I/O and accelerating its callbacks on the handle.
// This causes our related TaskCompletionSource objects to move to a terminal state.
//
// We only want to dispose the handle if we are actually waiting for a pending WinHTTP I/O to complete,
// meaning that we are await'ing for a Task to complete. While we could simply call dispose without
// a pending operation, it would cause random failures in the other threads when we expect a valid handle.
private void CancelPendingResponseStreamReadOperation()
{
WinHttpTraceHelper.Trace("WinHttpResponseStream.CancelPendingResponseStreamReadOperation");
lock (_state.Lock)
{
if (_state.AsyncReadInProgress)
{
WinHttpTraceHelper.Trace("WinHttpResponseStream.CancelPendingResponseStreamReadOperation: before dispose");
_requestHandle?.Dispose(); // null check necessary to handle race condition between stream disposal and cancellation
WinHttpTraceHelper.Trace("WinHttpResponseStream.CancelPendingResponseStreamReadOperation: after dispose");
}
}
} |
|
@davidsh @stephentoub This commit dotnet/corefx@48214d7 is the biggest departure from 1.1.0. It could've introduced (or enhanced) the issue. Would you recommend to test the previous version of the code or a workaround switch to non-winhttp version of HttpClientHandler? |
|
Moving this discussion to offline email communication... |
|
I'm suddenly getting the same errors on a reverse proxy component set of unit tests. Google gets me here for that stacktrace, but you closed the issue, and i'm unsure as to the resolution on this ticket. Should I add to this ticket, open another one or is there another ticket tracking this? Error is at https://ci.appveyor.com/project/OpenRasta/openrasta-core/build/2.6.0-preview.1.1298%2Bmaster |
|
Please open a new issue. Please attach a repro so that it can be diagnosed. It may not be same root cause as this original issue. |
|
kk |
This PR addresses some error handling problems in WinHttpHandler as well as improves the error messages generated for WinHttpException. This was root caused to a bug in WinHttp where it can't do a GET request with a request body using chunked encoding. However, this exception was masked due to an error handling bug caused by an inconsistency in how WinHttp associates context values for callbacks during the WinHttpSendRequest API call. This PR now fixes the error handling around synchronous errors being returned from WinHttpSendRequest. The root WinHttp bug itself can't be fixed. So, doing a GET request will still throw an exception, but it will be a catchable HttpRequestException. Another bug was discovered while investigating #26278. WinHttpCloseHandle should only be called once and should never race between threads. This is normally protected when the request handle is closed via SafeHandle Dispose(). But there was a place in the callback method where we called WinHttpCloseHandle directly against the raw handle. Finally, the error messages for WinHttpExceptions have been improved to show the error number and the probable WinHttp API call that generated the error. This will save diagnostic time. I also moved the WinHttpException source code back from Common. It was originally shared between WinHttpHandler and ClientWebSocket. But then ClientWebSocket moved away from WinHttp implementation. So, it makes more sense for this class to be under WinHttpHandler. Example: Original WinHttpException message: "The parameter is incorrect" New WinHttpException message: "Error 87 calling WinHttpSendRequest, 'The parameter is incorrect'." Closes #28156 Contributes to #26278
ghost
locked as resolved and limited conversation to collaborators
We are reporting daily crashes after moving our application from .Net 4.6 to .Net Core 2.0. When the crash occurs all 64 Cores are spiked at 100% CPU.
Example Call Stack:
dotnet/corefx#1: SafeWinHttpHandle.ReleaseHandle()
dotnet/corefx#2: SafeWinHttpHandle.ReleaseHandle()
dotnet/corefx#3: WinHttpHandler.HandleAsyncException ()
dotnet/corefx#4
Some Notes on the usage of
HttpClient:HttpClientHandlerasstatic readonly HttpMessageHandlerpublic object Invoke(string method, Type returnType = null, object parameters = null)public async Task<object> InvokeAsync(string method, Type returnType = null, object parameters = null, CancellationToken cancellationToken = default (CancellationToken))async Task<object> GetResponseAsyncHttpClienton every call as newHttpClient(webServerInvoker.Handler, false)wherewebServerInvoker.Handleris the shared static instance.PostAsyncresult, a timedCancellationTokenis provided .HttpClientinstance at the end of the call.[EDIT] Formatting changes by @karelz
The text was updated successfully, but these errors were encountered: