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

Handle poorly-behaved callbacks more reliably #636

Merged
merged 2 commits into from
Dec 4, 2020

Conversation

staticfloat
Copy link
Contributor

If startwrite() is never called during a server callback, the client
may be left waiting for a response and never get one. This can cause
lockups and general misery, especially when connections are proxied
through an intermediary (such as nginx) with a finite connection
limit.

This PR detects a server callback that never calls startwrite(http)
and throws an error, triggering the HTTP 500 mechanism. It also adds a
closewrite(http) onto that branch as without it, it appears that the
final chunk in the Transfer-Encoding: Chunked response may not appear.

If `startwrite()` is never called during a server callback, the client
may be left waiting for a response and never get one.  This can cause
lockups and general misery, especially when connections are proxied
through an intermediary (such as `nginx`) with a finite connection
limit.

This PR detects a server callback that never calls `startwrite(http)`
and throws an error, triggering the HTTP 500 mechanism.  It also adds a
`closewrite(http)` onto that branch as without it, it appears that the
final chunk in the `Transfer-Encoding: Chunked` response may not appear.
@staticfloat
Copy link
Contributor Author

@quinnj is this the best way to detect a lack of startwrite(http) in the callback? I was halfway to adding an http.write_started flag that startwrite() would set, but then noticed that write() will conditionally call startwrite() automatically if !iswritable(http) is true, so I figured I'd follow that lead.

Also, I found that I needed to add a closewrite(http) to the error branch here to get the whole error message transmitted when chunked encoding is in effect. I'm not sure if I need to closeread() as well.

@staticfloat
Copy link
Contributor Author

Closes #631

@codecov-io
Copy link

codecov-io commented Dec 3, 2020

Codecov Report

Merging #636 (3cfa43c) into master (7bf03e2) will decrease coverage by 1.99%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #636      +/-   ##
==========================================
- Coverage   77.46%   75.47%   -2.00%     
==========================================
  Files          36       36              
  Lines        2299     2145     -154     
==========================================
- Hits         1781     1619     -162     
- Misses        518      526       +8     
Impacted Files Coverage Δ
src/Servers.jl 64.62% <33.33%> (-2.27%) ⬇️
src/ConnectionPool.jl 72.59% <0.00%> (-5.04%) ⬇️
src/Handlers.jl 58.90% <0.00%> (-4.06%) ⬇️
src/multipart.jl 32.50% <0.00%> (-3.98%) ⬇️
src/ConnectionRequest.jl 48.07% <0.00%> (-3.71%) ⬇️
src/ExceptionRequest.jl 83.33% <0.00%> (-2.39%) ⬇️
src/RetryRequest.jl 52.63% <0.00%> (-2.37%) ⬇️
src/CookieRequest.jl 85.18% <0.00%> (-1.92%) ⬇️
src/parseutils.jl 71.42% <0.00%> (-1.91%) ⬇️
src/Pairs.jl 65.00% <0.00%> (-1.67%) ⬇️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7bf03e2...3cfa43c. Read the comment docs.

@quinnj
Copy link
Member

quinnj commented Dec 4, 2020

Nightly failures look unrelated; some Pkg error. This looks great to me; can I make one small request? Would you mind adding a big *NOTE: explanation of what's required in your own Stream handler to the docs here? I think that could possibly help the next poor traveler who stumbles along the way. Basically just that they should call startwrite/closewrite when handling their stream.

@quinnj quinnj merged commit 1a80e28 into JuliaWeb:master Dec 4, 2020
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.

3 participants