Workaround http.sys bug in ResponseBodyTests for Content-Length header #64960
Conversation
Bug filed on http.sys team.
github-actions
bot
added
the
area-networking
| string address; | ||
| using (Utilities.CreateHttpServer(out address, async httpContext => | ||
| { | ||
| httpContext.Features.Get<IHttpBodyControlFeature>().AllowSynchronousIO = true; |
There was a problem hiding this comment.
The PR description mentions a bug filed on the http.sys team and that this is a workaround, but there is no code comment, issue link, or documentation explaining what the http.sys bug is, why spaces in the Content-Length header value trigger it, or when this workaround can be removed. Consider adding a comment with a reference to the filed bug to help future maintainers understand why the spaces were removed.
| httpContext.Features.Get<IHttpBodyControlFeature>().AllowSynchronousIO = true; | |
| httpContext.Features.Get<IHttpBodyControlFeature>().AllowSynchronousIO = true; | |
| // This test intentionally uses a Content-Length value without any spaces. | |
| // There is a filed http.sys bug where Content-Length values containing spaces | |
| // can be parsed or handled incorrectly. The product code works around that bug | |
| // by normalizing/removing spaces before sending the header. This test verifies | |
| // that a clean Content-Length value is passed through unchanged by http.sys. | |
| // When the underlying http.sys bug is fixed and the workaround removed, this | |
| // test should be revisited to ensure behavior remains correct. |
| { | ||
| httpContext.Features.Get<IHttpBodyControlFeature>().AllowSynchronousIO = true; | ||
| httpContext.Response.Headers["Content-lenGth"] = " 30 "; | ||
| httpContext.Response.Headers["Content-lenGth"] = "30"; |
There was a problem hiding this comment.
The workaround for the http.sys bug has been applied only to this test method, but other test methods in the same file (lines 239, 253, 268, and 289) still use Content-Length header values with leading and trailing spaces. If this is a workaround for an http.sys bug, it should be consistently applied to all test methods that set the Content-Length header, or there should be a comment explaining why only this specific test needs the workaround.
|
Nice catch. I assume we don't intend to change this back once the Windows issue is fixed. |
|
No, I don't think we need to be responsible for testing edge cases in http.sys. |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Bug filed on http.sys team.