-
Notifications
You must be signed in to change notification settings - Fork 328
fix: close sockets on connection initialize timeout #762
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
Merged
tarcieri
merged 1 commit into
httprb:main
from
rpeng:rpeng/close-socket-on-timeout-in-initialize
Sep 26, 2023
Merged
fix: close sockets on connection initialize timeout #762
tarcieri
merged 1 commit into
httprb:main
from
rpeng:rpeng/close-socket-on-timeout-in-initialize
Sep 26, 2023
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6272bdb
to
c1ddb6f
Compare
c1ddb6f
to
5706556
Compare
rpeng
commented
Sep 22, 2023
require "io/wait" | ||
|
||
module HTTP | ||
module Timeout | ||
class Null | ||
extend Forwardable | ||
|
||
def_delegators :@socket, :close, :closed? |
There was a problem hiding this comment.
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
approved these changes
Sep 22, 2023
ixti
reviewed
Sep 26, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
netbsd-srcmastr
pushed a commit
to NetBSD/pkgsrc
that referenced
this pull request
Aug 11, 2025
Changelog: ## [5.3.1] - 2025-06-09 ### Changed - Revert switch to the native llhttp on MRI, as it's not compatible with standalone bundles ([#802](httprb/http#802)) ## [5.3.0] - 2025-06-09 ### Added - (backported) Add .retriable feature to Http - (backported) Add more specific ConnectionError classes - (backported) New feature: RaiseError ### Changed - (backported) Drop depenency on base64 - (backported) Cache header normalization to reduce object allocation - (backported) Use native llhttp on MRI ## [5.2.0] - 2024-02-05 ### Added - Add `Connection#finished_request?` ([#743](httprb/http#743)) - Add `Instrumentation#on_error` ([#746](httprb/http#746)) - Add `base64` dependency (suppresses warnings on Ruby 3.0) ([#759](httprb/http#759)) - Add `PURGE` HTTP verb ([#757](httprb/http#757)) - Add Ruby-3.3 support ### Changed - **BREAKING** Process features in reverse order ([#766](httprb/http#766)) - **BREAKING** Downcase Content-Type charset name ([#753](httprb/http#753)) - **BREAKING** Make URI normalization more conservative ([#758](httprb/http#758)) ### Fixed - Close sockets on initialize failure ([#762](httprb/http#762)) - Prevent CRLF injection due to broken URL normalizer ([#765](httprb/http#765)) [unreleased]: httprb/http@v5.3.0...5-x-stable [5.3.0]: httprb/http@v5.2.0...v5.3.0 [5.2.0]: httprb/http@v5.1.1...v5.2.0 ## 5.1.1 (2022-12-17) * [#731](httprb/http#731) Strip brackets from IPv6 addresses in `HTTP::URI`. ([@jeraki]) * [#722](httprb/http#722) Add `on_redirect` callback. ([@benubois]) ## 5.1.0 (2022-06-17) * Drop ruby-2.5 support. * [#715](httprb/http#715) Set default encoding to UTF-8 for `application/json`. ([@drwl]) * [#712](httprb/http#712) Recognize cookies set by redirect. ([@tkellogg]) * [#707](httprb/http#707) Distinguish connection timeouts. ([@YuLeven]) ## 5.0.4 (2021-10-07) * [#698](httprb/http#698) Fix `HTTP::Timeout::Global#connect_ssl`. ([@tarcieri]) ## 5.0.3 (2021-10-06) * [#695](httprb/http#695) Revert DNS resolving feature. ([@PhilCoggins]) * [#694](httprb/http#694) Fix cookies extraction. ([@flosacca]) ## 5.0.2 (2021-09-10) * [#686](httprb/http#686) Correctly reset the parser. ([@bryanp]) * [#684](httprb/http#684) Don't set Content-Length for GET, HEAD, DELETE, or CONNECT requests without a BODY. ([@jyn514]) * [#679](httprb/http#679) Use features on redirected requests. ([@nomis]) * [#678](httprb/http#678) Restore `HTTP::Response` `:uri` option for backwards compatibility. ([@schwern]) * [#676](httprb/http#676) Update addressable because of CVE-2021-32740. ([@matheussilvasantos]) * [#653](httprb/http#653) Avoid force encodings on frozen strings. ([@bvicenzo]) * [#638](httprb/http#638) DNS failover handling. ([@midnight-wonderer]) ## 5.0.1 (2021-06-26) * [#670](httprb/http#670) Revert `Response#parse` behavior introduced in [#540]. ([@DannyBen]) * [#669](httprb/http#669) Prevent bodies from being resubmitted when following unsafe redirects. ([@odinhb]) * [#664](httprb/http#664) Bump llhttp-ffi to 0.3.0. ([@bryanp]) ## 5.0.0 (2021-05-12) * [#656](httprb/http#656) Handle connection timeouts in `Features` ([@semenyukdmitry]) * [#651](httprb/http#651) Replace `http-parser` with `llhttp` ([@bryanp]) * [#647](httprb/http#647) Add support for `MKCALENDAR` HTTP verb ([@meanphil]) * [#632](httprb/http#632) Respect the SSL context's `verify_hostname` value ([@colemannugent]) * [#625](httprb/http#625) Fix inflator with empty responses ([@LukaszMaslej]) * [#599](httprb/http#599) Allow passing `HTTP::FormData::{Multipart,UrlEncoded}` object directly. ([@ixti]) * [#593](httprb/http#593) [#592](httprb/http#592) Support informational (1XX) responses. ([@ixti]) * [#590](httprb/http#590) [#589](httprb/http#589) Fix response headers paring. ([@Bonias]) * [#587](httprb/http#587) [#585](httprb/http#585) Fix redirections when server responds with multiple Location headers. ([@ixti]) * [#581](httprb/http#581) [#582](httprb/http#582) Add Ruby 2.7.x support. ([@janko]) * [#577](httprb/http#577) Fix `Chainable#timeout` with frozen Hash. ([@antonvolkoff]) * [#576](httprb/http#576) [#524](httprb/http#524) **BREAKING CHANGE** Preserve header names casing. ([@joshuaflanagan]) * [#540](httprb/http#540) [#538](httprb/http#538) **BREAKING CHANGE** Require explicit MIME type for Response#parse ([@ixti]) * [#532](httprb/http#532) Fix pipes support in request bodies. ([@ixti]) * [#530](httprb/http#530) Improve header fields name/value validation. ([@Bonias]) * [#506](httprb/http#506) [#521](httprb/http#521) Skip auto-deflate when there is no body. ([@Bonias]) * [#489](httprb/http#489) Fix HTTP parser. ([@ixti], [@fxposter]) * [#546](httprb/http#546) **BREAKING CHANGE** Provide initiating `HTTP::Request` object on `HTTP::Response`. ([@joshuaflanagan]) * [#571](httprb/http#571) Drop Ruby 2.3.x support. ([@ixti]) * [3ed0c31](httprb/http@3ed0c31) Drop Ruby 2.4.x support.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
Simultaneously:
You'll notice that sockets will stay in ESTABLISHED or CLOSE_WAIT as you make more and more requests that time out.
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)