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

Fix: Handle client connection reset for keep-alive connections #156

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tisba
Copy link

@tisba tisba commented Nov 10, 2024

See #86

TCPSocket#eof? will raise Errno::ECONNRESET if the connection has been reset. This is currently not handled properly in WEBrick::HTTPServer#run. This was reported by @carlhoerberg, but incorrectly addressed in #86.

Preconditions to hit this issue:

  • Client has to request with Connection: keep-alive
  • WEBrick has to respect that (e.g. errors / 404s do close the connection)
  • Client resets the connection after the first request

This will leave the server believe that the connection is still good and fail in WEBrick::HTTPServer#run when testing TCPSocket#eof? in the next iteration of #run.

The fix is to rescue Errno::ECONNRESET when checking for #eof?.

Testing this is a little tricky, here is how I approached it:

  • start a test server, using WEBrick::HTTPServer
  • setup a success response on /
  • do a single request with Connection: keep-alive
  • after reading the response body, fish the TCPSocket object out of the Net::HTTP instance and configure it to linger on close and forcefully reset the connection, dropping all remaining data. This will send a RST to the server
  • to avoid TestWEBrick shutting down, before we hit the issue, we need to wait a little bit

@tisba
Copy link
Author

tisba commented Nov 18, 2024

Can somebody kick of the test workflow? @olleolleolle maybe? :)

Copy link
Contributor

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

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

Thank you for working on this! The approach you used for fixing the issue seems best to me.

@tisba
Copy link
Author

tisba commented Dec 13, 2024

Got the error again yesterday (using webrick a lot in little local projects) and got reminded of this: Is there anything left to be done here for me, @jeremyevans?

@jeremyevans
Copy link
Contributor

@tisba Nothing left for you to do. I've approved this, so another developer should just need to review and merge it. I would recommend against using webrick, though. Is there a reason you couldn't use puma?

@tisba
Copy link
Author

tisba commented Dec 13, 2024

Let me rephrase and clarify: Anything that I work on and is production-grade uses puma. But I have some very tiny toy projects where webrick is great, because no native extensions, minimal dependencies etc. I also still use an alias to ruby -run -e httpd . -p 9090 almost daily – old habits just die hard :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants