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: close sockets on connection initialize timeout #762

Merged

Conversation

rpeng
Copy link
Contributor

@rpeng rpeng commented Sep 22, 2023

When timeouts are set on the client, there is potential for a resource leak when timeouts occur during connection initialization.

When timeout errors are thrown during connection initialization, the connection object itself is not created (nil) and the client won't be able to properly close the connection.

This causes the created socket to stay in either an ESTABLISHED or CLOSE_WAIT state, while the client no longer has a reference to it, effectively causing a leak.

Steps to repro:

In the client:

# irb or rails c
client = HTTP.timeout(5)

client.get("https://www.example.org").body.to_s # or use 240.0.0.0
client.get("https://www.example.org").body.to_s
client.get("https://www.example.org").body.to_s

Simultaneously:

  1. Set up a mitm to intercept example.org, and add network conditioning to make the request very slow. Or you can use a black hole IP such as 240.0.0.0
  2. Check the sockets opened by the ruby process:
watch -n 1 "lsof -i -P | grep PROCESS_PID" # replace PROCESS_PID with the rails process pid

You'll notice that sockets will stay in ESTABLISHED or CLOSE_WAIT as you make more and more requests that time out.

ruby    28497 rpeng   21u  IPv4 43444771      0t0  TCP f6eb5410737c:57558->93.184.216.34:443 (CLOSE_WAIT)
ruby    28497 rpeng   23u  IPv4 43449275      0t0  TCP f6eb5410737c:56876->93.184.216.34:443 (CLOSE_WAIT)
ruby    28497 rpeng   24u  IPv4 43452649      0t0  TCP f6eb5410737c:34242->93.184.216.34:443 (ESTABLISHED)
ruby    28497 rpeng   25u  IPv4 43458906      0t0  TCP f6eb5410737c:55366->93.184.216.34:443 (ESTABLISHED)
ruby    28497 rpeng   26u  IPv4 43461133      0t0  TCP f6eb5410737c:37510->93.184.216.34:443 (ESTABLISHED)

The fix ensures that a socket is closed if any timeouts happen during initialization, though I suspect we could make this even more stringent (e.g. on any StandardError, we ensure that we send @socket.close if a socket exists and is open)

@rpeng rpeng force-pushed the rpeng/close-socket-on-timeout-in-initialize branch 2 times, most recently from 6272bdb to c1ddb6f Compare September 22, 2023 05:07
require "io/wait"

module HTTP
module Timeout
class Null
extend Forwardable

def_delegators :@socket, :close, :closed?
Copy link
Contributor Author

@rpeng rpeng Sep 22, 2023

Choose a reason for hiding this comment

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

Forwardable and def_delegators don't really work with safe nav operators

@tarcieri tarcieri requested a review from ixti September 22, 2023 12:56
@tarcieri tarcieri merged commit 3b7133c into httprb:main Sep 26, 2023
8 checks passed
@rpeng rpeng deleted the rpeng/close-socket-on-timeout-in-initialize branch September 26, 2023 17:13
Copy link
Member

@ixti ixti left a comment

Choose a reason for hiding this comment

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

LGTM

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.

None yet

3 participants