search

nicolasff / webdis

A Redis HTTP interface with JSON output
https://webd.is
BSD 2-Clause "Simplified" License
2.84k stars 304 forks source link

Only process `Connection: close` header if full request was read (fixes #194) #195

Closed jessie-murray closed 3 years ago

jessie-murray commented 3 years ago

The presence of a Connection: close header sets a flag on the request, which we process by no longer running the HTTP parser as soon as it returns from parsing a chunk. Since HTTP 100 uploads require at least 2 reads, we need to handle the Connection: close only if we've reached the end of the request. This change sets a flag in on_message_complete, which we use in combination with the header to stop reading.

Also adding a test to reproduce this issue and including it in CI job.

@nicolasff I tried to find an existing flag but didn't find anything already set in http_client_on_message_complete that I could use. There's c->keep_alive = 0 just above the flag I set, but it's 0 by default due to the calloc so I don't think it's a reliable indicator that the request has been fully parsed. I was also thinking there might be something inside the parser object itself but I didn't really want to rely on its internals (what if it changes later?). But if you have a better idea it might avoid this 1-byte increase, even though this is very small and might actually not make any difference due to alignment.w

jessie-murray commented 3 years ago

force-push is adding curl to the CI job due to:

./tests/curl-tests.sh: line 7: curl: command not found
Error: Process completed with exit code 127.
nicolasff commented 3 years ago

Nice! Thanks for the PR. I looked at http_parser and couldn't find a field that indicates that parsing has ended; the callback is triggered from multiple places and it seems better to set a flag explicitly.

On a related topic, I filed #196 to look at http_parser which hasn't been touched in years.