-
Notifications
You must be signed in to change notification settings - Fork 7
[:httpd] Fix HTTP large request + response handling #14
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
base: master
Are you sure you want to change the base?
Conversation
UncleGrumpy
left a comment
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.
|
Also, we don't have any CI checks for this repo yet, but would you please signoff all of your commits? |
|
I could squash + sign when we are done with review? i can also open another PR with the commits signed |
Since this PR is focused on just the one bug fix, squash and sign will be great. No need to open a separate PR. I would have helped you get previous commits signed if that was necessary. ;-) |
…te management Signed-off-by: harmon25 <dougwright1@gmail.com>
Signed-off-by: harmon25 <dougwright1@gmail.com>
Signed-off-by: harmon25 <dougwright1@gmail.com>
|
OK i signed off with a force push! |
src/gen_tcp_server.erl
Outdated
| addr => any | ||
| }). | ||
| -define(DEFAULT_SOCKET_OPTIONS, #{}). | ||
| -define(MAX_SEND_CHUNK, 2048). |
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.
I wonder if it would be better to set this a bit smaller, like 1024, so that it is less than most network MTU size (typically 1350-1500, depending on network). Exceeding the MTU will cause packet fragmentation, which isn't much of a problem for unix machines, but may not be handled as gracefully by microcontrollers.
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.
in my independent fork i landed on 1460
https://github.com/harmon25/atomvm_httpd/blob/diagnostics/src/gen_tcp_server.erl#L49-L51
i will try and port those changes here also
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.
That makes sense. 1500 is the most common MTU, and I think there is some tcp/ip overhead in each packet on top of what httpd is sending. If a VPN is used the MTU will be lower. I think 2048 is too high. We should actually expose this as a configuration option, with some lower default (like 1460 that you are using).
…r - incorporated from harmon25/atomvm_httpd Signed-off-by: harmon25 <dougwright1@gmail.com>
Signed-off-by: harmon25 <dougwright1@gmail.com>
|
This breaks iolist response bodies: My response tuple contains a |
…iodata) Signed-off-by: harmon25 <dougwright1@gmail.com>
…ng frame parsing logic Signed-off-by: harmon25 <dougwright1@gmail.com>
Signed-off-by: harmon25 <dougwright1@gmail.com>
|
All of the changes here I have mirrored in my standalone fork - which adds a bunch of tests via ex_unit - i have also ported over a change that allows for websocket frames that span multiple packets, as well as the ability to pass socket options through a new Sorry this PR got a bit out of hand...I wont add anything else but suggested fixes - will also test with the examples here to validate |
|
untill i can validate the original example app i will keep this in draft - still seem to have issues... |
Copilot assisted - might not be cleanest implementation of a fix for the issue, but it does the trick.