feat: opt-in HTTP keep-alive via keep_alive_connections#145
Conversation
Each FCM call currently opens a fresh TCP/TLS connection via Faraday.default_adapter. For high-volume senders this is the dominant per-request cost. Add an opt-in keep_alive_connections http_option that reuses a thread-local Faraday connection (per uri) backed by net-http-persistent, so the handshake and HTTP/2 stream are amortised across requests. - Default is false: existing callers see no behaviour change. - Connections are cached per (thread, uri) keyed by object_id so multiple FCM clients in the same process do not share sockets, and Net::HTTP's non-thread-safety is respected. - Bearer token and per-call extra_headers are re-applied each yield because JWTs expire and project_id headers vary across calls. - On any error the cached connection is dropped — a half-closed socket would just fail again on retry. - keep_alive_idle_timeout_seconds (default 30) and keep_alive_pool_size (default 1) are configurable. Pattern mirrors the typesense-ruby fork (PR decision-labs#55). Prompt: "Lets do lib/fcm tweak on fcm forked repo olio/fcm, I already contributor so we can create PR there" Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
| # pool_size defaults to 1: we already cache one Faraday connection per | ||
| # (thread, uri), and Net::HTTP is not thread-safe — so a single socket | ||
| # per pool is the safe default. Override only with a specific reason. | ||
| faraday.adapter :net_http_persistent, pool_size: @keep_alive_pool_size do |http| |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [86/80]
| request: { timeout: @http_options.fetch(:timeout, DEFAULT_TIMEOUT) } | ||
| ) do |faraday| | ||
| # pool_size defaults to 1: we already cache one Faraday connection per | ||
| # (thread, uri), and Net::HTTP is not thread-safe — so a single socket |
There was a problem hiding this comment.
Style/AsciiComments: Use only ascii symbols in comments.
| def apply_default_headers(connection, extra_headers) | ||
| connection.headers["Content-Type"] = "application/json" | ||
| connection.headers["Authorization"] = "Bearer #{jwt_token}" | ||
| connection.headers["access_token_auth"] = "true" |
There was a problem hiding this comment.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
|
|
||
| def apply_default_headers(connection, extra_headers) | ||
| connection.headers["Content-Type"] = "application/json" | ||
| connection.headers["Authorization"] = "Bearer #{jwt_token}" |
There was a problem hiding this comment.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
| end | ||
|
|
||
| def apply_default_headers(connection, extra_headers) | ||
| connection.headers["Content-Type"] = "application/json" |
There was a problem hiding this comment.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
| stub_request(:post, uri).to_raise(Faraday::ConnectionFailed.new('boom')) | ||
|
|
||
| expect { client.send_v1(send_v1_params) }.to raise_error(Faraday::ConnectionFailed) | ||
| expect(client.__send__(:thread_connections)).not_to have_key(FCM::BASE_URI_V1) |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [84/80]
|
|
||
| stub_request(:post, uri).to_raise(Faraday::ConnectionFailed.new('boom')) | ||
|
|
||
| expect { client.send_v1(send_v1_params) }.to raise_error(Faraday::ConnectionFailed) |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [89/80]
|
|
||
| it 'discards the cached connection when a request raises' do | ||
| client.send_v1(send_v1_params) | ||
| expect(client.__send__(:thread_connections)[FCM::BASE_URI_V1]).to be_a(Faraday::Connection) |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [97/80]
| stub_request(:post, uri).to_return(body: '{}', headers: {}, status: 200) | ||
| end | ||
|
|
||
| it 'caches a Faraday connection per (thread, uri) and reuses it across calls' do |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [84/80]
| describe 'keep_alive_connections' do | ||
| let(:client) { FCM.new(json_key_path, project_name, keep_alive_connections: true) } | ||
| let(:uri) { "#{FCM::BASE_URI_V1}#{project_name}/messages:send" } | ||
| let(:send_v1_params) { { 'token' => 'token', 'notification' => { 'title' => 'hi' } } } |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [90/80]
|
@sabman we have been testing this on production some time now and we can see 20-30ms request time benefits on hot paths, as it is optional maybe okay to merge? |
|
@erimicel sure let me quickly review today |
2 participants
Summary
Each FCM call currently opens a fresh TCP/TLS connection via
Faraday.default_adapter. For high-volume senders this is the dominant per-request cost. This PR adds an opt-inkeep_alive_connectionshttp_optionthat reuses a thread-local Faraday connection (per uri) backed bynet-http-persistent, so the handshake and HTTP/2 stream are amortised across requests.Design
false— existing callers see no behaviour change. The originalFaraday.default_adapterone-shot path is preserved.(thread, uri)cache keyed byobject_id, so multiple FCM clients in the same process do not share sockets andNet::HTTP's non-thread-safety is respected.extra_headersare re-applied each yield because JWTs expire (~1h) andproject_idheaders vary across calls.require "faraday/net_http_persistent"is only loaded when keep-alive is enabled, so existing users do not pull in the extra dependency at runtime.Test plan