Skip to content

Reuse persistent HTTP connections via Faraday + net-http-persistent - #2

Open
allen12921 wants to merge 1 commit into
masterfrom
feature/persistent-http-connection
Open

Reuse persistent HTTP connections via Faraday + net-http-persistent#2
allen12921 wants to merge 1 commit into
masterfrom
feature/persistent-http-connection

Conversation

@allen12921

Copy link
Copy Markdown

Every call went through RestClient::Request.execute, opening a fresh TCP+TLS connection per request, adding avoidable latency and limiting concurrency under load.

Switches the transport to Faraday backed by faraday-net_http_persistent (net-http-persistent under the hood), which pools keep-alive connections per host. Net::HTTP is what Semian's semian/net_http patch instruments, so consumers that wrap WeChat calls in a Semian circuit breaker keep that protection; an Excon-based adapter tried earlier this session bypassed it silently and was reverted before publishing.

  • get_request keeps its prior default behavior (verify_ssl: false, TLSv1_2, 30s timeout, GET-redirect following) and gains explicit support for headers/proxy/SSL version bounds, validating unknown option keys instead of silently misusing them (this is what caught a :method/:payload misuse in a downstream caller during rollout).

  • Connections are cached per thread and per (SSL config, proxy) so two threads, or two proxy configurations, never share one adapter's mutable timeout/proxy state.

  • A disabled (nil) timeout is mapped to a 1-day ceiling rather than passed straight through, since Faraday only assigns a timeout onto the persistent connection when the value is truthy and a bare nil would leave the previous call's timeout in place.

  • Redirect limit raised to 10 and schemeless URLs are normalized to http://, matching rest-client's prior defaults.

  • Adds post_request, sharing option validation, timeout mapping, proxy, headers, and connection selection with get_request via a new perform_request helper, so downstream callers needing POST (creating a scene QR code, batch-tagging users) get the same connection reuse instead of bypassing this gem with a bare RestClient::Request.execute. payload is JSON-encoded unless already a String; Content-Type is set to application/json unless the caller's own :headers overrides it. A dedicated NO_PAYLOAD sentinel (compared by identity) is used instead of payload's truthiness, so a caller can still send a literal JSON false or null body through post_request.

  • Raises the pooled connection's idle_timeout above net-http-persistent's 5s default: real WeChat traffic (a user scanning a QR code, a form submission) arrives tens of seconds to minutes apart, so at the 5s default the connection was closed and re-established on almost every call anyway, and pooling added no measurable benefit. faraday-net_http_persistent 1.2 (pinned to match this app's faraday ~> 1.10) doesn't expose idle_timeout as a connection option; 2.x does but requires faraday ~> 2.5. Rather than force that upgrade, a small adapter subclass sets idle_timeout once, right after the connection is first built. If the server closes it before that window elapses anyway, net-http-persistent already retries on a fresh connection, already covered by an existing test.

  • Caches the per-thread connection pool via Thread#thread_variable_get/set instead of Thread.current[]/[]=, which is fiber-local rather than thread-local and would otherwise hand every request on a fiber-based server a fresh, unpooled connection, defeating the reuse this change exists for.

  • Only forwards ssl_ca_file to net-http-persistent when verify_ssl resolves true: net-http-persistent forces verify_mode back to VERIFY_PEER whenever ca_file is set, so passing it through unconditionally silently re-enabled verification for callers that had explicitly disabled it.

  • Documents (rather than fixes) a known gap: Faraday::Adapter::NetHttp #call configures the shared Net::HTTP::Persistent manager (SSL, open/read/write timeout, proxy) and only afterwards dispatches through it, as two separate steps; on a Fiber-scheduler-based server, many fibers on one thread would share this same manager, and a fiber switch landing between "configure" and "dispatch" could let one fiber's request go out with settings a second fiber configured in between. A Mutex around the whole call was tried and reverted: under a real Fiber scheduler, a second fiber locking a Mutex already held by another fiber on the same thread raises ThreadError (deadlock) instead of waiting - worse than the race it was meant to prevent. Fixing this properly needs fiber-scoped adapters sharing one underlying socket pool; accepted as a gap since nothing indicates this gem runs under a Fiber-scheduler server today.

  • Stops tracking the gem build artifact left in the working tree and ignores root-level *.gem files, so a future gem build doesn't get bundled into spec.files via git ls-files and embedded in subsequent releases.

  • Raises required_ruby_version to >= 3.3.1 (was >= 2.6) and moves CI to that version. net-http-persistent's own dependency range allows connection_pool < 4, and connection_pool 3.0.2 uses anonymous ** kwarg forwarding that requires Ruby >= 3.2 - a resolution this gem could previously land a caller on while still claiming Ruby 2.6+ support. Raising the floor past what every one of this branch's dependencies needs removes that mismatch instead of pinning around it.

  • reset_connections! now shuts down each cached connection's underlying Net::HTTP::Persistent manager before dropping it. Faraday::Adapter#close is a no-op by default and faraday-net_http_persistent doesn't override it, so simply clearing the cache (as before) left pooled sockets open until an eventual GC; the test suite alone calls reset_connections! once per test.

  • IdleTimeoutAdapter restores max_retries to 1 after Faraday's own configure_request unconditionally zeroes it on every call. Ruby's Net::HTTP (since 2.5) uses max_retries to transparently reopen and retry an idempotent request exactly once if the server had already closed the socket; Faraday assumes a fresh connection is unlikely to already be dead, but that assumption doesn't hold for a pooled, reused one, which is the whole point of this adapter.

  • Pins connection_pool >= 2.5.5, confirmed (by inspecting installed gem sources) to be the version that added automatic post-fork connection invalidation; net-http-persistent's own dependency only requires >= 2.2.4, which would let an app already locked to an older connection_pool have a forked worker reuse the parent's inherited socket.

  • Extracts userinfo from a request URL (https://user:pass@host/x) and sends it as HTTP Basic auth, as rest-client did. Faraday only does this for a connection's url_prefix, never for a per-request URL, which is all get_request/post_request ever pass it, so a credentialed URL was silently making an unauthenticated request.

  • Raises Faraday::ClientError for a 3xx status faraday-follow_redirects doesn't itself follow (300, 304, 305, 306 - it handles 301/302/303/ 307/308). Faraday::Response::RaiseError only raises on 4xx/5xx, so these were reaching JSON.parse as if successful; rest-client raised for any status outside what it returned or redirected on.

  • Adds StrictFollowRedirects, a small subclass of faraday-follow_redirects' middleware that also requires a non-blank Location header before following a redirect. Without it, a 3xx response missing Location resolves to the same URL and gets silently repeated (the same JSON body, for a POST) up to the redirect limit, where rest-client raised immediately on the first such response.

  • Leaves one rest-client behavior deliberately unmatched: it carried Set-Cookie values from a redirect response into the redirected request via a cookie jar (the http-cookie gem); this doesn't. api.weixin.qq.com is a stateless JSON API with no occasion to set a cookie a redirect target would need, and matching this exactly would mean adding faraday-cookie_jar/http-cookie as new runtime dependencies for it - not worth it for this gap. Documented in code.

  • Widens the status check to reject any response outside 200..207 (was: any 3xx not already redirected). rest-client only ever treated 200..207 as success; an obscure 2xx like 208 (Already Reported) or 226 (IM Used) would otherwise still reach JSON.parse as if successful, since Faraday's raise_error middleware only raises on 4xx/5xx.

  • Adds base64 as a direct runtime dependency. Faraday::Request::BasicAuthentication (used for the URL-embedded- credentials fix above) requires it; base64 is a default gem bundled with the interpreter through Ruby 3.3 but not from 3.4 on, and neither this gem nor faraday 1.10 declared it, which could raise LoadError on 3.4+ for an authenticated URL.

  • Stringifies :headers option values before merging them into the request. rest-client stringified header values, so a Symbol (e.g. accept: :json) or a number was a supported shorthand; Net::HTTP calls #strip on the raw header value, so passing one through unconverted now raises NoMethodError instead of sending it as rest-client did. Leaves one part of that unmatched, deliberately: rest-client also expanded a MIME shorthand like :json to "application/json" via the mime-types gem; this only stringifies it to the literal "json". Not worth a new runtime dependency for content negotiation this gem's fixed WeChat endpoints never do. Documented in code.

  • StrictFollowRedirects also rejects 308 (Permanent Redirect) and, for 301/302/307, any method other than GET/HEAD. faraday-follow_redirects treats 308 like 307 and otherwise follows 301/302/307/308 for any method (301/302 silently become GET; 307/308 replay the original body); rest-client only ever followed 301/302/307 for GET/HEAD, never followed 308 at all, and raised in every other case. Without this, a post_request hitting a 301/302/307 could have its POST silently turned into a GET or replayed, duplicating a state-changing call.

  • Treats a -1 open_timeout/read_timeout/write_timeout the same as nil (disabled). rest-client accepted -1 as a deprecated alias for nil on open_timeout/read_timeout; passed straight through, Net::HTTP eventually uses it as a negative IO wait interval and raises ArgumentError instead of disabling the timeout.

Every call went through RestClient::Request.execute, opening a fresh
TCP+TLS connection per request, adding avoidable latency and limiting
concurrency under load.

Switches the transport to Faraday backed by faraday-net_http_persistent
(net-http-persistent under the hood), which pools keep-alive connections
per host. Net::HTTP is what Semian's semian/net_http patch instruments,
so consumers that wrap WeChat calls in a Semian circuit breaker keep that
protection; an Excon-based adapter tried earlier this session bypassed it
silently and was reverted before publishing.

- get_request keeps its prior default behavior (verify_ssl: false,
  TLSv1_2, 30s timeout, GET-redirect following) and gains explicit
  support for headers/proxy/SSL version bounds, validating unknown
  option keys instead of silently misusing them (this is what caught a
  :method/:payload misuse in a downstream caller during rollout).
- Connections are cached per thread and per (SSL config, proxy) so two
  threads, or two proxy configurations, never share one adapter's
  mutable timeout/proxy state.
- A disabled (nil) timeout is mapped to a 1-day ceiling rather than
  passed straight through, since Faraday only assigns a timeout onto
  the persistent connection when the value is truthy and a bare nil
  would leave the previous call's timeout in place.
- Redirect limit raised to 10 and schemeless URLs are normalized to
  http://, matching rest-client's prior defaults.
- Adds post_request, sharing option validation, timeout mapping, proxy,
  headers, and connection selection with get_request via a new
  perform_request helper, so downstream callers needing POST (creating
  a scene QR code, batch-tagging users) get the same connection reuse
  instead of bypassing this gem with a bare RestClient::Request.execute.
  payload is JSON-encoded unless already a String; Content-Type is set
  to application/json unless the caller's own :headers overrides it. A
  dedicated NO_PAYLOAD sentinel (compared by identity) is used instead
  of payload's truthiness, so a caller can still send a literal JSON
  false or null body through post_request.
- Raises the pooled connection's idle_timeout above
  net-http-persistent's 5s default: real WeChat traffic (a user
  scanning a QR code, a form submission) arrives tens of seconds to
  minutes apart, so at the 5s default the connection was closed and
  re-established on almost every call anyway, and pooling added no
  measurable benefit. faraday-net_http_persistent 1.2 (pinned to match
  this app's faraday ~> 1.10) doesn't expose idle_timeout as a
  connection option; 2.x does but requires faraday ~> 2.5. Rather than
  force that upgrade, a small adapter subclass sets idle_timeout once,
  right after the connection is first built. If the server closes it
  before that window elapses anyway, net-http-persistent already
  retries on a fresh connection, already covered by an existing test.
- Caches the per-thread connection pool via Thread#thread_variable_get/set
  instead of Thread.current[]/[]=, which is fiber-local rather than
  thread-local and would otherwise hand every request on a fiber-based
  server a fresh, unpooled connection, defeating the reuse this change
  exists for.
- Only forwards ssl_ca_file to net-http-persistent when verify_ssl
  resolves true: net-http-persistent forces verify_mode back to
  VERIFY_PEER whenever ca_file is set, so passing it through
  unconditionally silently re-enabled verification for callers that had
  explicitly disabled it.
- Documents (rather than fixes) a known gap: Faraday::Adapter::NetHttp
  #call configures the shared Net::HTTP::Persistent manager (SSL,
  open/read/write timeout, proxy) and only afterwards dispatches
  through it, as two separate steps; on a Fiber-scheduler-based server,
  many fibers on one thread would share this same manager, and a fiber
  switch landing between "configure" and "dispatch" could let one
  fiber's request go out with settings a second fiber configured in
  between. A Mutex around the whole call was tried and reverted: under
  a real Fiber scheduler, a second fiber locking a Mutex already held
  by another fiber on the same thread raises ThreadError (deadlock)
  instead of waiting - worse than the race it was meant to prevent.
  Fixing this properly needs fiber-scoped adapters sharing one
  underlying socket pool; accepted as a gap since nothing indicates
  this gem runs under a Fiber-scheduler server today.
- Stops tracking the gem build artifact left in the working tree and
  ignores root-level *.gem files, so a future `gem build` doesn't get
  bundled into spec.files via git ls-files and embedded in subsequent
  releases.
- Raises required_ruby_version to >= 3.3.1 (was >= 2.6) and moves CI to
  that version. net-http-persistent's own dependency range allows
  connection_pool < 4, and connection_pool 3.0.2 uses anonymous **
  kwarg forwarding that requires Ruby >= 3.2 - a resolution this gem
  could previously land a caller on while still claiming Ruby 2.6+
  support. Raising the floor past what every one of this branch's
  dependencies needs removes that mismatch instead of pinning around
  it.
- reset_connections! now shuts down each cached connection's underlying
  Net::HTTP::Persistent manager before dropping it. Faraday::Adapter#close
  is a no-op by default and faraday-net_http_persistent doesn't override
  it, so simply clearing the cache (as before) left pooled sockets open
  until an eventual GC; the test suite alone calls reset_connections!
  once per test.
- IdleTimeoutAdapter restores max_retries to 1 after Faraday's own
  configure_request unconditionally zeroes it on every call. Ruby's
  Net::HTTP (since 2.5) uses max_retries to transparently reopen and
  retry an idempotent request exactly once if the server had already
  closed the socket; Faraday assumes a fresh connection is unlikely to
  already be dead, but that assumption doesn't hold for a pooled,
  reused one, which is the whole point of this adapter.
- Pins connection_pool >= 2.5.5, confirmed (by inspecting installed
  gem sources) to be the version that added automatic post-fork
  connection invalidation; net-http-persistent's own dependency only
  requires >= 2.2.4, which would let an app already locked to an older
  connection_pool have a forked worker reuse the parent's inherited
  socket.
- Extracts userinfo from a request URL (https://user:pass@host/x) and
  sends it as HTTP Basic auth, as rest-client did. Faraday only does
  this for a connection's url_prefix, never for a per-request URL,
  which is all get_request/post_request ever pass it, so a credentialed
  URL was silently making an unauthenticated request.
- Raises Faraday::ClientError for a 3xx status faraday-follow_redirects
  doesn't itself follow (300, 304, 305, 306 - it handles 301/302/303/
  307/308). Faraday::Response::RaiseError only raises on 4xx/5xx, so
  these were reaching JSON.parse as if successful; rest-client raised
  for any status outside what it returned or redirected on.
- Adds StrictFollowRedirects, a small subclass of
  faraday-follow_redirects' middleware that also requires a non-blank
  Location header before following a redirect. Without it, a 3xx
  response missing Location resolves to the same URL and gets silently
  repeated (the same JSON body, for a POST) up to the redirect limit,
  where rest-client raised immediately on the first such response.
- Leaves one rest-client behavior deliberately unmatched: it carried
  Set-Cookie values from a redirect response into the redirected
  request via a cookie jar (the http-cookie gem); this doesn't.
  api.weixin.qq.com is a stateless JSON API with no occasion to set a
  cookie a redirect target would need, and matching this exactly would
  mean adding faraday-cookie_jar/http-cookie as new runtime
  dependencies for it - not worth it for this gap. Documented in code.
- Widens the status check to reject any response outside 200..207
  (was: any 3xx not already redirected). rest-client only ever treated
  200..207 as success; an obscure 2xx like 208 (Already Reported) or
  226 (IM Used) would otherwise still reach JSON.parse as if
  successful, since Faraday's raise_error middleware only raises on
  4xx/5xx.
- Adds base64 as a direct runtime dependency.
  Faraday::Request::BasicAuthentication (used for the URL-embedded-
  credentials fix above) requires it; base64 is a default gem bundled
  with the interpreter through Ruby 3.3 but not from 3.4 on, and
  neither this gem nor faraday 1.10 declared it, which could raise
  LoadError on 3.4+ for an authenticated URL.
- Stringifies :headers option values before merging them into the
  request. rest-client stringified header values, so a Symbol (e.g.
  accept: :json) or a number was a supported shorthand; Net::HTTP calls
  #strip on the raw header value, so passing one through unconverted
  now raises NoMethodError instead of sending it as rest-client did.
  Leaves one part of that unmatched, deliberately: rest-client also
  expanded a MIME shorthand like :json to "application/json" via the
  mime-types gem; this only stringifies it to the literal "json".
  Not worth a new runtime dependency for content negotiation this
  gem's fixed WeChat endpoints never do. Documented in code.
- StrictFollowRedirects also rejects 308 (Permanent Redirect) and, for
  301/302/307, any method other than GET/HEAD. faraday-follow_redirects
  treats 308 like 307 and otherwise follows 301/302/307/308 for any
  method (301/302 silently become GET; 307/308 replay the original
  body); rest-client only ever followed 301/302/307 for GET/HEAD, never
  followed 308 at all, and raised in every other case. Without this, a
  post_request hitting a 301/302/307 could have its POST silently
  turned into a GET or replayed, duplicating a state-changing call.

- Treats a -1 open_timeout/read_timeout/write_timeout the same as nil
  (disabled). rest-client accepted -1 as a deprecated alias for nil on
  open_timeout/read_timeout; passed straight through, Net::HTTP
  eventually uses it as a negative IO wait interval and raises
  ArgumentError instead of disabling the timeout.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@warmwind warmwind left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  1. Undocumented breaking changes. Errors are now Faraday::* instead of RestClient::*, and Semian's Net::CircuitOpenError arrives wrapped in Faraday::ConnectionFailed (Faraday rescues Net::ProtocolError), so callers that rescue by class silently stop catching them. The description says Semian protection is preserved, but only the instrumentation is. Dropping the rest-client dependency, requiring Ruby >= 3.3.1, and raising ArgumentError on unknown options are also breaking, and none of it is in the README.

  2. GET read timeouts are retried. IdleTimeoutAdapter#configure_request sets max_retries = 1, which also re-sends a GET on Net::ReadTimeout: with timeout: 0.5 the call took 1.01s and the server received 2 requests (POST is not retried). This matches rest-client, but test_it_should_enforce_read_timeout_over_https only asserts < 3s, which hides it.

  3. Too much surface for the use case. A 10-line method became ~230 lines plus two subclasses, largely to replicate rest-client edge cases the fixed WeChat endpoints never hit (URL userinfo as Basic auth, schemeless URLs, 208/226/308, -1 timeouts, Symbol header values). Comments are long and narrate history ("A Mutex around the whole call was tried and reverted"), and the commit message refers to "earlier this session".

  4. Depends on private internals. IdleTimeoutAdapter overrides net_http_connection / configure_request and reads @cached_connection. shutdown_persistent_manager walks @app in an until loop that never terminates if it reaches nil.

  5. Version constraints. faraday ~> 1.10 blocks consumers from upgrading to Faraday 2. required_ruby_version >= 3.3.1 is stricter than the stated reason needs (connection_pool 3.x requires Ruby 3.2).

  6. Minor. The description says a built .gem is no longer tracked, but the diff removes no file and only edits .gitignore. The README doesn't mention post_request or the supported options.

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