Skip to content
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

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

Merged

Conversation

jessie-murray
Copy link
Collaborator

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 jessie-murray requested a review from nicolasff May 9, 2021 23:02
…ff#194)

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.
@jessie-murray jessie-murray force-pushed the http-100-with-connection-close branch from 6efaa6f to 3c67055 Compare May 9, 2021 23:04
@jessie-murray
Copy link
Collaborator Author

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
Copy link
Owner

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.

@nicolasff nicolasff merged commit 6556039 into nicolasff:master May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants