Fix HTTP parsing on MacOs
hellxcz opened this issue · 25 comments
I tried to create a simple HTTP service with simple GET method handle.
On MacOS there is a different NewLine convention ("\r\n"). Handling of such situation is present in code in RequestParser code, unfortunately dotNet String.IndexOf("\n", ..) does not work properly. Always returns -1 as index on MacOS.
Simple change from "\n":string to '\n':char fixes the issue.
Thanks for reporting the issue.
However I don't get quite well what is the issue. Is there an exception, handler not responding to the request or something else? Can you reproduce the problem with an UT?
Hi.
it fails on bad parsing of HTTP request.
On MacOS the new line is "\r\n". And when you would try to find the "\n" in such string, dotNet will return -1 as not found.
Consequence of this is, that parsing of HTTP does not work at all.
I found, that simple change of finding the '\n' char works well, is culture invariant and does not break existing logic of finding "\r". Its strange behaviour of dotNet core on mac and may be not-reproducible on windows or linux machine.
Do you have access to intel Apple machine ?
I have Mac and everything developed here was on Mac. I didn’t see any problem before. It has to be culture specific then. Without your change is there any test failing? Could you reproduce the failure with a UT so I can look at it?
I found this, when it was running with small sample and I was calling it over postman.
It is reproducible, when in ResourceTestFixtures i changed the NewLineDelimiter to "\r\n". Than all tests, which are dependent on RequestParser started to fail.
Hi Vaugn.
I dont want to be evil. Naming is not maybe correct, but it referred to ResourceTestFixtures, where fixture messages for test runs are defined. When those messages are changed to contain "\r\n", it should reproduce issue. On my machine it did.
Hi Vaugn.
I dont want to be evil. Naming is not maybe correct, but it referred to ResourceTestFixtures, where fixture messages for test runs are defined. When those messages are changed to contain "\r\n", it should reproduce issue. On my machine it did.
I just don't get how your PR #6 fix the issue. The thing you've done is replacing inlined \n with the same thing from a variable.
You mean that actually the code is not handling properly \r\n but the PR is not fixing that point ?
That NewLineDelimiter in RequestParser (not in ResourceTestFixtures, where it is string for convenient way to replace delimiter in test messages) is of type char with value of '\n'. Looks the same, but when it is string, as original code, the String.IndexOf method will not find string "\n" properly, when delimiter is of value "\r\n". Actually it will not find it at all. Thats odd thing in dotNet core. Changing it to char '\n' will fix this behaviour and starts to work.
Ok, I understand. I'll give it a try.
If the line ends with CRLF, that's two characters. I don't understand how attempting to find the single character LF can't succeed. The single characters CR and LF are in the string. You should be able to find any of CR or LF or CRLF.
What am I missing?
I was also confused and spent some time with debugging, because it does not make too much sense. But it fixed broken behaviour with real running app. Simple one, but debugged with real calls from postman.
well, I found it. Its a strange stuff in .net 5.
check out this fiddle - https://dotnetfiddle.net/mMyDyz
when it's changed to .net 4.xxx both works fine.
Wow. Looks like you found a bug on .NET side 👍🏻
Man, that was not a purpose :D
Are you using Framework or Core? We use Core, and I think Core 3 (or 2). @tjaskula ?
@VaughnVernon it's .NET 5 now. Core is replaced slowly by it
That was .net 5. there will be no Core version.
LOL, there's no way for me to keep up with Microsoft's every move.
- .NET Framework
- .NET Standard & .NET Core
- .NET Core
- .NET
- .???
.NET 5 is new implementation of .NET Core and Framework. Cross platform which wipes out all previous.
Makes sense. I'll try to remember.
However this '\n' parsing string is a glitch for me. An issue should be opened there :)
Guys, I'm sorry for bothering. This really seems like weird stuff on .NET 5 side. Who would expect such thing ?
Question is, if Microsoft dotNet team will fix this, or if they will present this as new feature, that needs to be manually migrated.
Thanks for nice discussion.
Given that a missing LF or CRLF is an HTTP error, should you try up to two times as follows?
if (Find(CRLF)) use line
else if (Find(LF)) use line
else error
@hellxcz No bothering at all. I'm glad you reported the issue. At least we know there is a problem.
Addressed in 380816e...7b2d9c6