Deadlock on returning a non-async stream from SocketsHttpHandler.ConnectCallback #6740
Comments
|
Issue moved from dotnet/docs#23960
From @adegeo on Friday, May 7, 2021 8:23:31 PM If it's related to documentation, do you have the specific article that is misrepresenting how this should work? If it's a bug with .NET, this issue should probably be reported on https://github.com/dotnet/sdk/issues Would you like me to transfer the issue there? |
|
Issue moved from dotnet/docs#23960
From @jhudsoncedaron on Friday, May 7, 2021 8:26:25 PM @adegeo : Point being, the API surface of ConnectCallback says return a Stream and the default implementation of Stream.*Async() just forwards to the *Sync() functions; so it should be assumed to work unless documented otherwise. Yet it does deadlock if given a stream that doesn't override *Async() to actually be async and no documentation says so. |
|
Issue moved from dotnet/docs#23960
From @adegeo on Friday, May 7, 2021 8:34:07 PM Ahh OK. I'll transfer this to the api docs repo. |
|
Issue moved from dotnet/docs#23960
From @adegeo on Friday, May 7, 2021 8:45:35 PM Well apparently I don't have rights to that repo 😢 @gewarren can you do it? |
|
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label. |
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsIssue moved from dotnet/docs#23960
From @jhudsoncedaron on Tuesday, April 27, 2021 5:52:49 PM Not sure if this is an issue in the code or the documentation. Issue description On returning a non-async-capable stream from SocketsHttpHandler.ConnectCallback, HttpClient deadlocked. I don't think a toy example will reproduce; it got through several requests before getting stuck. HttpClient assumes that whatever stream it gets is completely async-capable which is in fact true of NetworkStream but is not true of arbitrary streams. There is no indication in the documentation that a completely async-capable stream must be returned, and it is surprising that it deadlocks. Normally it's safe for an async I/O call to become sync; it's just slower. HttpClient has a different opinion. While I haven't a reason* to run HttpClient through any of these, there are streams for which the only reasonable implementation of ReadAsync() just calls Read() and is synchronous and if you really need an async read you have to sacrifice another thread. Also, the default implementation of ReadAsync() in IO.Stream does indeed call Read(). *I needed to force HttpClient to be sync when it refused to be due to another bug elsewhere. Target framework
|
|
@jhudsoncedaron Do you know where it is deadlocking? |
|
@geoffkizer : It tries to call ReadAsync on a background thread, which turns into Read() and deadlocks in the kernel because the call to Write() that starts the next request never starts while the Read() is pending. |
|
Are you saying you've implemented ReadAsync on your stream to synchronously block? If so, that's a violation of the contract, and much more than just HttpClient will have issues with it. ReadAsync can perform work synchronously, but it shouldn't block synchronously indefinitely waiting for I/O. You say "the default implementation of Stream.*Async() just forwards to the *Sync() functions", but they do so asynchronously by queuing, not by invoking Read/Write synchronously as part of the call to Read/WriteAsync. If that's not the issue, please share a simple repro to help further explain the concern. Thanks. |
|
@stephentoub: It's still going to try to start Write() while that background Read() is still pending. Putting it on a background thread doesn't help. The Write() will wait for the Read() to finish before starting, but there won't be any more data because the other end is waiting for the Write() before it sends anything back. |
Are you talking about the semaphore in the base Stream that serializes the async-over-sync operations? |
|
I'm actually talking about the in-kernel semaphore that blocks the second synchronous API call on the first one finishing first. |
|
I don't understand. If you're saying that in your own implementation of Read and Write the Read requires Write to be called to wake it up but the implementation (at whatever level) prevents the Write from running while the Read is pending, how do you expect this to work? That's got nothing to do with HttpClient. If you were talking about the semaphore used by Stream to serialize the synchronous operations queued by the base implementations of Read/WriteAsync, that's covered by dotnet/runtime#45089. |
|
Given this in the documetation for ReadFile() I didn't expect anybody to ever attempt an async I/O call they weren't prepared to deal with the consequence of it becoming synchronous: "If a file or device is opened for asynchronous I/O, subsequent calls to functions such as ReadFile using that handle generally return immediately, but can also behave synchronously with respect to blocked execution. For more information see http://support.microsoft.com/kb/156932." There's actually a few more cases than listed in the accompanying kb article. This means that no implementation of ReadAsync() that isn't ultimately backed by a socket can actually guarantee the call is asynchronous, and therefore no caller should depend on it being asynchronous. |
Where do you see code depending on it being asynchronous? If it completes synchronously, great, the task returned from ReadAsync will already be complete by the time it's handed back. I do not understand the concern. Please share a minimal repro of the problem you're facing. |
|
How about if it completes synchronously three minutes later? The call might become a synchronous call that is blocked. |
Huh? If you're using the base Stream.ReadAsync implementation, then from the perspective of the caller it's still an asynchronous operation. There's a thread pool thread somewhere that would be blocked for three minutes, after which that asynchronous operation would complete. Again, I do not understand the concern. Please share a minimal repro of the problem you're facing. |
|
All I had to do was fail to implement ReadAsync() and WriteAsync() in a trivial wrapper and it deadlocked on the second request. |
Yes, this is because of the Stream's semaphore I mentioned in #6740 (comment) and that is covered by dotnet/runtime#45089. Stream's base implementation of ReadAsync/WriteAsync serialize operations. |
|
Closing as a dupe of dotnet/runtime#45089. |
Issue moved from dotnet/docs#23960
From @jhudsoncedaron on Tuesday, April 27, 2021 5:52:49 PM
Not sure if this is an issue in the code or the documentation.
Issue description
On returning a non-async-capable stream from SocketsHttpHandler.ConnectCallback, HttpClient deadlocked. I don't think a toy example will reproduce; it got through several requests before getting stuck. HttpClient assumes that whatever stream it gets is completely async-capable which is in fact true of NetworkStream but is not true of arbitrary streams. There is no indication in the documentation that a completely async-capable stream must be returned, and it is surprising that it deadlocks. Normally it's safe for an async I/O call to become sync; it's just slower. HttpClient has a different opinion.
While I haven't a reason* to run HttpClient through any of these, there are streams for which the only reasonable implementation of ReadAsync() just calls Read() and is synchronous and if you really need an async read you have to sacrifice another thread. Also, the default implementation of ReadAsync() in IO.Stream does indeed call Read().
*I needed to force HttpClient to be sync when it refused to be due to another bug elsewhere.
Target framework
The text was updated successfully, but these errors were encountered: