Several more HttpListener fixes on Unix #20280
Conversation
|
This must feel pretty satisfying - LGTM. One nit: there's quite a lot of hardcoded strings for headers. Maybe we could use HttpKnownHeaderNames or whatever it's called? |
| _context.ErrorMessage = "Invalid request line (version)."; | ||
| return; | ||
| } | ||
| if (_version != HttpVersion.Version10 && _version != HttpVersion.Version11) |
There was a problem hiding this comment.
Do we have tests for 1.* versions? E.g 1.2, 1.3
There was a problem hiding this comment.
E.g 1.2, 1.3
There is no such thing as those versions. There is only HTTP/1.0, HTTP/1.1 and HTTP/2.0. Although I don't think HttpListener supports HTTP/2.0 at all. And it is a transport protocol anyways for which HTTP/1.1 is carried over on it.
There was a problem hiding this comment.
I know those versions don't exist, but HttpListener still accepts them. This test passes on Windows, used to pass with the managed implementation, but now fails after this PR
[Theory]
[InlineData("1.2")]
[InlineData("1.3")]
[InlineData("1.4")]
[InlineData("1.5")]
[InlineData("1.6")]
[InlineData("1.7")]
[InlineData("1.8")]
[InlineData("1.9")]
public void Test(string version)
{
using (Socket client = Factory.GetConnectedSocket())
{
client.Send(Factory.GetContent(version, "POST", null, "Hi", null, false));
var ctx = Factory.GetListener().GetContext();
Assert.Equal(new Version(version), ctx.Request.ProtocolVersion);
}
}There was a problem hiding this comment.
@stephentoub you can bring this commit into your branch to test it:
https://github.com/hughbe/corefx/commit/aa3489c991f26e8ddb91b50c8ba265b7145163ac
There's no httpsys ID to copy, so just create a new one.
stephentoub
force-pushed
the
more_listener_fixes
branch
from
e0cae0e
to
096ef77
Compare
| // [ActiveIssue(20232, TestPlatforms.AnyUnix)] [InlineData("POST", "Content-Length: 9223372036854775808", 0)] | ||
| [InlineData("POST", "Content-Length: 9223372036854775807", 9223372036854775807)] // long.MaxValue | ||
| [InlineData("POST", "Content-Length: 9223372036854775808", 0)] // long.MaxValue + 1 | ||
| [InlineData("POST", "Content-Length: 18446744073709551615 ", 0)] // ulong.MaxValue |
There was a problem hiding this comment.
Should we also add ulong.MaxValue + 1 to make sure nothing every overflows?
[InlineData("POST", "Content-Length: 18446744073709551616 ", 0)] // ulong.MaxValue + 1There was a problem hiding this comment.
Not with this test; as written it'll hang, on both Windows and Unix.
It only currently passes on Windows because the Windows implementation actually does some work, making it unlikely that the test will fail, but still possible. On Unix, the implementation is a stub and completes synchronously, thereby always failing.
stephentoub
force-pushed
the
more_listener_fixes
branch
from
65b94ca
to
a936515
Compare
Simply reuse the Windows implementation's cookie handling, moving it to a shared file, calling it from the managed, and removing the managed's implementation.
stephentoub
force-pushed
the
more_listener_fixes
branch
from
a936515
to
2416022
Compare
Replaces #20265, which was having CI issues.
Fixes #20239
Fixes #20238
Fixes #20237
Fixes #20232
Fixes #20231
Fixes #20230
Fixes #20229
Fixes #20228
Fixes #20165
Fixes #20101
Fixes #20100
Fixes #20099
Fixes #19977
Fixes #19972