-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add support for Transfer-Encoding: chunked #16636
Conversation
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.
Thank you for this!
I left some comments, please also include info about how you tested this locally (i.e. a test plan).
I've been testing locally with my repo above, though the shortest possible test case I've used is:
and then running If there's a way to include this kind of curl testing in the nim tests, I'd be happy to do that too. |
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.
If there's a way to include this kind of curl testing in the nim tests, I'd be happy to do that too.
What might be the best way to test it is to capture the data that curl sends, then just create a simple Nim script that does the following:
- Creates an asynchttpserver instance
- sends the data that curl would send using a raw socket (you can use newAsyncSocket() + connect + send for this)
As I mentioned in the comment I think you need to test this more thoroughly manually as well. I will merge this anyway though, we can always make fixes to the broken implementation without any problems. The main thing for me is that the API either doesn't change or is changed in a way that makes sense :)
|
||
# Read bytesToRead and add to body | ||
# Note we add +2 because the line must be terminated by \r\n | ||
let chunk = await client.recv(bytesToRead + 2) |
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.
This looks suspicious to me, I have a feeling that chunked encoding doesn't delimit data with \r\n. You should test with larger files to make sure they work (if you're just testing with "hello world" then you're unlikely to run into this).
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.
@dom96 The two sources I've found with details on this topic say that the chunks are delimited with \r\n
, see:
- (MDN): https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Transfer-Encoding
- (Wikipedia): https://en.wikipedia.org/wiki/Chunked_transfer_encoding#Example
However, you were right to be suspicious! The code needs to discard these newlines, which I did not correctly do. I've fixed this (and added tests!) in this follow-up PR: #16678
I'd appreciate it if you could take a look and review that too! Thanks!
Also thank you for going the extra mile to resolve my comments, I saw that you had some trouble with the converter in IRC. The reason it's there is for backwards compatibility: so that you don't need to change |
* Add support for Transfer-Encoding: chunked * Minor whitespace fixes * Use recv instead of recvLineInto * Undo changes to httpcore, inline changes
I've spent the past few days trying to use Nim with OpenFaas (my first Nim project, just a proof-of-concept, see: https://gitlab.com/vabresto/nim-sudoku-faas/-/tree/master/) and had some issues with the Nim HTTP server because the OpenFaas of-watchdog proxy doesn't pass the
Content-Length
header, but rather uses chunked encoding (see: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Transfer-Encoding).I've added support for this for my own uses and wanted to contribute it back to Nim.
I'd like to add tests but I'm not sure what the best way to do so is. Additionally, I can't actually compile and run the devel branch locally (OSX 15.6), so I made these changes based on the Nim 1.4 code I have locally.
Any feedback would be very welcome. Thanks!