-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Fix ConnectCallback_UseNamedPipe_Success test #89748
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsCloses #73772 The main issue seems to be that the A minimal change here could be to just add a
|
} | ||
} | ||
} | ||
await using Http2LoopbackConnection loopbackConnection = await Http2LoopbackConnection.CreateAsync(socket: null, serverStream, new Http2Options { UseSsl = useSsl }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use the Generic version so we have ability to test both Http 1.1 and 2?
I think adding the Dispose may be still worth or awe may add explicit method to disconnect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTTP/1.1 doesn't work over named pipes when using TLS -- the same issue as #73097 with HTTP/2.
The server is finishing its SslStream handshake and issues a WriteAsync
call.
The client (HttpClient) finished its handshake and is now sending the request headers. It calls WriteAsync
.
Neither write is finishing because the other side is not reading.
And each side won't read until the write goes through.
I added a workaround for that in the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTTP/1.1 doesn't work over named pipes when using TLS
Is this something we should track? cc @JamesNK in case it is a problem for dotnet/aspnetcore#14207
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does work if at least one side has pending reads to unblock the other.
This is not something we do in HttpClient/our loopback server, but this test now works around.
If I'm looking at the right part of Kestrel code for named pipes, they do have such logic (similar to their socket transport). So I'd imagine this should work with Kestrel (but I haven't tested it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect that the sever should always try to read the request, right? Perhaps is is shortcoming of out loopback code...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, the server got stuck before even getting to the request. It was still trying to do the final write in the SslStream handshake.
So I guess in Kestrel's case it would depend on whether they're buffering the stream before/after SslStream.
If it's after they'd be stuck in the handshake the same way our loopback server would be. If it's before, it should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Closes #73772
This test was only disabled for
IsNativeAot
, but I can repro the failure locally.The main issue seems to be that the
loopbackConnection.DisposeAsync
is waiting for the client to disconnect, but the client will only disconnect once the loopback stream is disposed.A minimal change here could be to just add a
client.Dispose
at the end of the inner block, but while I was looking into this I just rewrote it into our more common shape of separating out the client and server.