-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
IIS HTTP/2 Response Trailers #24120
IIS HTTP/2 Response Trailers #24120
Conversation
src/Servers/IIS/IIS/test/testassets/InProcessWebSite/Startup.cs
Outdated
Show resolved
Hide resolved
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.
Just a minor comment in the unit tests. Otherwise LGTM!
See https://github.com/dotnet/aspnetcore-internal/issues/3170 for details of bringing up new queues. I believe with have 20H1 builds now, but I think this still needs something even newer? |
src/Servers/IIS/AspNetCoreModuleV2/InProcessRequestHandler/managedexports.cpp
Outdated
Show resolved
Hide resolved
src/Servers/IIS/AspNetCoreModuleV2/InProcessRequestHandler/managedexports.cpp
Outdated
Show resolved
Hide resolved
@@ -169,6 +169,11 @@ private async Task WriteBody(bool flush = false) | |||
// if request is done no need to flush, http.sys would do it for us | |||
if (result.IsCompleted) |
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.
When is the reader completed? Is it always after the request pipeline exits? Or can CompleteAsync make it complete early?
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.
The reader is completed after the request pipeline exits: https://github.com/dotnet/aspnetcore/blob/master/src/Servers/IIS/IIS/src/Core/IISHttpContextOfT.cs#L93. It can also be completed due to client disconnect or Abort.
CompleteAsync currently doesn't make it complete early: https://github.com/dotnet/aspnetcore/blob/master/src/Servers/IIS/IIS/src/Core/IISHttpContext.FeatureCollection.cs
src/Servers/IIS/IIS/test/Common.FunctionalTests/Inprocess/ResponseTrailersTests.cs
Show resolved
Hide resolved
src/Servers/IIS/IIS/test/testassets/InProcessWebSite/Startup.cs
Outdated
Show resolved
Hide resolved
Writing skip conditions will be hard, I somehow need to know if the iiscore.dll is updated to a new enough version... |
16ed8e6
to
6343ac6
Compare
Wouldn't that be in sync with the OS version? We have skips for that. |
Yeah, that would work. I think I need to skip entirely for now as there isn't a version of windows that has that yet. |
namespace Microsoft.AspNetCore.Server.IIS.FunctionalTests | ||
{ | ||
[Collection(PublishedSitesCollection.Name)] | ||
public class ResponseTrailersTests : IISFunctionalTestBase |
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.
Please add a test that combines trailers with CompleteAsync.
Check that CompleteAsync immediately sends trailers to the client and ends the response while the request delegate is still running.
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.
Those are the tests we had to remove, CompleteAsync doesn't work in IIS's request lifetime model. Is that going to be a problem? Maybe for the streaming error case?
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.
CompleteAsync is used with the deadline timeout. When the timeout is reached the timer calls CompleteAsync to send trailing HEADERS frame, then sends RST_STREAM frame with either IHttpResetFeature/HttpContext.Abort.
What will happen in IIS?
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.
The reset will probably happen first and prevent the trailers from being sent.
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.
FYI Justin is implementing reset now so we should be able to verify that.
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.
Will the reset/abort happen immediately (by which I mean will the server send RST_STREAM straight away) or will it wait for the request delegate to complete?
Would properly supporting CompleteAsync require changes in IIS? How big a change would it be?
Does HttpSys properly support CompleteAsync?
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.
HttpSys does properly support CompleteAsync.
Justin has some ideas for how to implement CompleteAsync in IIS. There's a related issue over at #17268. I think it can be done without changes to IIS, but it would require a lot of hardening to AspNetCore to avoid touching any native IIS structures after CompletAsync.
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.
Ok! To give you an idea of what the impact on gRPC would be:
If CompleteAsync isn't supported correctly, and the server doesn't send trailers to the client, there is a chance that gRPC calls that exceed the deadline timeout will fail with the wrong status code. I say chance because there is always a race between the client and server to report the exceeded deadline. If the client wins then the right status will be reported, if the server wins then the wrong status will be reported.
This wouldn't be disastrous to gRPC support on IIS for 5.0, but it would be a known issue that would have to be called out. And we'd want to try to fix it in the future.
src/Servers/IIS/AspNetCoreModuleV2/InProcessRequestHandler/managedexports.cpp
Show resolved
Hide resolved
Updated based on latest changes from IIS layer. |
1f65fb7
to
2b9839b
Compare
2b9839b
to
26c0f3b
Compare
Adds support for IIS Response Trailers. cc @pgovind @GrabYourPitchforks @Tratcher
Noteworthy: