Replace StreamAsyncHelper with StreamApmExtensions in System.Net.Security #7800
Conversation
|
Wonderful, wonderful, wonderful... 😁 🚀 |
|
|
||
| if (task == null || ar == null) | ||
| { | ||
| throw new ArgumentException(SR.InvalidOperation_WrongAsyncResultOrEndReadCalledMultiple); |
There was a problem hiding this comment.
Can the resources be deleted as well, or are they still used elsewhere in the assembly?
There was a problem hiding this comment.
Yup, deleted, thanks.
…rity System.Net.Security was originally written to use Stream.Begin/EndRead/Write. When it was ported to .NET Core, it took a dependency on a helper that provided such Begin/End methods, but this helper was originally written for a different purpose: providing async Begin/End wrappers for the synchronous Read/Write methods. As a result of this mismatch, the async calls in System.Net.Security are queueing work items that then do work synchronously, blocking thread pool threads unnecessarily. This commit changes those helpers to sit on top of ReadAsync/WriteAsync instead. Eventually System.Net.Security should have its async I/O redone to sit natively on ReadAsync/WriteAsync using async/await, as it'll result in less overhead, but for now with minimal changes this improves the scalability of the library.
stephentoub
force-pushed
the
netsecurity_streamhelpers
branch
from
0fca070 to
6a84b6d
Compare
|
LGTM |
joshfree
added
the
blocking
|
@ericstj @weshaggard PTAL : I remember there were some corner cases that StreamAsyncHelper was potentially handling (which is why it was recommended initially). Thanks @stephentoub! LGTM |
|
@stephentoub issue on full framework is Checking coreclr |
|
Works on coreclr! 😄 |
|
Issue on full framework is it use the frameworks SSL stream which has the issue. This version is only compatible with net46 and not net451; is it possible for it to target 451? |
|
What's the underlying stream being wrapped by SslStream? |
|
Its a custom stream that implements sync and async https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNetCore.Server.Kestrel/Http/FrameResponseStream.cs Actually its read/write so likely is this one first https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNetCore.Server.Kestrel/Http/FrameDuplexStream.cs Which has BeginWrite and EndWrite defined in 451; in addition to ReadAsync+WriteAsync |
|
Thanks, @benaadams. The problem is that the .NET Framework code for SslStream is written on top of the Begin/End methods, but you can't override the Begin/End methods as they're not exposed in the contracts, so SslStream ends up using the base Stream.Begin/EndXx methods, which a) just queue work items to call the synchronous methods, and b) because those synchronous methods are rarely implemented to be used concurrently uses a semaphore to ensure access is serialized. I'm discussing options offline with some folks, e.g. changing the .NET Framework implementation of Begin/End, adding the Begin/End methods back to the contracts, etc. |
|
Would using corefx's version of SslStream be an option or would that not help? |
It would address this particular case of the problem, but not other cases (other types consuming the Begin/End methods). I also don't think it would be feasible, unless all of the code were cloned/renamed into ASP.NET's implementation, which has its own obvious and significant downsides. |
|
Related issue aspnet/KestrelHttpServer#768 |
…reamhelpers Replace StreamAsyncHelper with StreamApmExtensions in System.Net.Security Commit migrated from dotnet/corefx@d10541f
System.Net.Security was originally written to use Stream.Begin/EndRead/Write. When it was ported to .NET Core, it took a dependency on a helper that provided such Begin/End methods, but this helper was originally written for a different purpose: providing async Begin/End wrappers for the synchronous Read/Write methods. As a result of this mismatch, the async calls in System.Net.Security are queueing work items that then do work synchronously, blocking thread pool threads unnecessarily. This commit changes those helpers to sit on top of ReadAsync/WriteAsync instead. Eventually System.Net.Security should have its async I/O redone to sit natively on ReadAsync/WriteAsync using async/await, as it'll result in less overhead, but for now with minimal changes this improves the scalability of the library.
cc: @davidsh, @CIPop, @ericeil, @mikeharder