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

Streams: don't error when writing closing bytes #546

Merged
merged 1 commit into from
Nov 19, 2020

Conversation

fonsp
Copy link
Member

@fonsp fonsp commented Jun 16, 2020

A common error when using Firefox on localhost is:

┌ Error: error handling request
│   exception =
│    IOError: stream is closed or unusable
│    Stacktrace:
│     [1] check_open at ./stream.jl:328 [inlined]
│     [2] uv_write_async(::Sockets.TCPSocket, ::Ptr{UInt8}, ::UInt64) at ./stream.jl:961
│     [3] uv_write(::Sockets.TCPSocket, ::Ptr{UInt8}, ::UInt64) at ./stream.jl:924
│     [4] unsafe_write(::Sockets.TCPSocket, ::Ptr{UInt8}, ::UInt64) at ./stream.jl:1007
│     [5] unsafe_write at /home/fons/.julia/packages/HTTP/GkPBm/src/ConnectionPool.jl:174 [inlined]
│     [6] macro expansion at ./gcutils.jl:91 [inlined]
│     [7] write at ./strings/io.jl:186 [inlined]
│     [8] closebody at /home/fons/.julia/packages/HTTP/GkPBm/src/Streams.jl:113 [inlined]
│     [9] closewrite(::HTTP.Streams.Stream{HTTP.Messages.Request,HTTP.ConnectionPool.Transaction{Sockets.TCPSocket}}) at /home/fons/.julia/packages/HTTP/GkPBm/src/Streams.jl:128
│     [10] (::HTTP.Servers.var"#13#14"{HTTP.Handlers.var"#4#5"{HTTP.Handlers.StreamHandlerFunction{Pluto.var"#71#75"}},HTTP.ConnectionPool.Transaction{Sockets.TCPSocket},HTTP.Streams.Stream{HTTP.Messages.Request,HTTP.ConnectionPool.Transaction{Sockets.TCPSocket}}})() at ./task.jl:333
└ @ HTTP.Servers ~/.julia/packages/HTTP/GkPBm/src/Servers.jl:373

Which led to lots of confusing error messages when using Pluto.jl. I believe the error is due to Firefox closing the IO before receiving the bytes 0\r\n\r\n when navigating to another page.

This PR simply wraps the closing write call in a try catch block - I believe that this only affects the situation where a clean close is not possible, but both ends already know that the connection is closing. (I should point out that I am not familiar with the HTTP protocol!)

@fonsp
Copy link
Member Author

fonsp commented Jun 16, 2020

This should fix #392 at HTTP.jl (and linked issues like waralex/Dashboards.jl#18 and fonsp/Pluto.jl#123) Those errors are about starting a connection - this PR only fixes the closing case

@fonsp
Copy link
Member Author

fonsp commented Jul 4, 2020

I'm not sure why the tests failed, can I do anything to help with this PR?

@fonsp
Copy link
Member Author

fonsp commented Nov 13, 2020

ping

@quinnj quinnj closed this Nov 19, 2020
@quinnj quinnj reopened this Nov 19, 2020
Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm ok with this; seems mostly harmless. If there's some other writing error, we usually throw, but in this case, we've already sent the "content" of the request, so whether or not these final bytes get through seems inconsequential, reinforced by the fact that, empirically, browsers seem to not care either.

The number of empty try-catches throughout the request code kind of pains my soul though. The code is getting ugly and is probably due for a larger refactor/cleanup.

@quinnj quinnj merged commit 98cba5d into JuliaWeb:master Nov 19, 2020
@fonsp
Copy link
Member Author

fonsp commented Nov 19, 2020

Thank you! I can see why you were reluctant to merge this PR then :)

@fonsp fonsp deleted the patch-1 branch November 19, 2020 10:15
@clarkevans
Copy link
Contributor

@quinnj Generally, a write error just isn't something a web service can do anything about. It means the connection or route went away, user closed browser, closed a tab, navigated away, lost their wifi signal, etc. Perhaps it might be nice to track some statistics about it... in the aggregate they might be interesting (but your reverse proxy will know about them), applications simply can't be concerned with these -- they are basically not actionable.

So, if you were going to refactor, you could make a function an application could override, but just set its body to nothing by default. Let it compile into a noop.

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