feat: enable HTTP/2 and restore keep-alive in Node transport#3
Conversation
amix
force-pushed
the
amix/http2-and-keepalive
branch
from
7482c2f to
2abffb1
Compare
|
Might be worth getting @goncalossilva's view on this too, as he added the dispatcher stuff initially into the SDKs. |
Backs up the "h2 multiplexing replaces /batch" claim from #2 for Node consumers (browsers already negotiate h2 automatically). Without these, Node consumers — including the MCP — were stuck on HTTP/1.1 with near-disabled keep-alive, so every request opened a fresh TCP+TLS connection. Changes: - `EnvHttpProxyAgent({ allowH2: true })` so ALPN can pick h2 when the server offers it (Comms via CloudFront does; fallback to h1.1 is automatic when it doesn't). - Drop the 1ms keepAliveTimeout/keepAliveMaxTimeout hack; use undici defaults so the connection pool actually persists across requests. - Wrap the full dispatcher construction in `suppressExperimentalWarningsSync` so the `allowH2` experimental warning is swallowed alongside the decompress one. - Add `closeDefaultDispatcher()` (exported from the package root) and `CommsApi.close()` so short-lived CLIs / scripts — including ones that only use the OAuth helpers — can drain the pool before exit. Validated direction with expert-letter (Claude + Codex) and Doistbot. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
amix
force-pushed
the
amix/http2-and-keepalive
branch
from
2abffb1 to
9ec9bd1
Compare
doistbot
left a comment
doistbot
left a comment
There was a problem hiding this comment.
This PR enables HTTP/2 multiplexing and restores proper keep-alive behavior in the Node transport while providing a mechanism to drain the connection pool before exit. These changes significantly improve transport efficiency for Node consumers by eliminating constant TCP/TLS handshakes. There are a few architectural and concurrency details to refine, specifically resolving a race condition during shutdown, clarifying the global versus instance-level scope of the dispatcher, and strengthening the related test coverage.
|
I also wonder if we should patch todoist-sdk-typescript? I don’t think these HTTP hacks we do are optimal; we should really just adopt HTTP/2 🤔 |
Clear the singleton before awaiting init. Previously, `await defaultDispatcherPromise` yielded to the event loop with the singleton still set; a concurrent `getDefaultDispatcher()` could grab the old dispatcher reference and then have it torn down underneath it. Per Doistbot review on #3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Does the Todoist backend support HTTP/2 then? |
|
@scottlovegrove yep, it does 👍 This will have pretty big impact on REST endpoints |
|
I don't have a strong opinion here, apart from preferring defaults whenever possible (so no code/config whenever possible). Every change we make adds more code on top. 😅 But it must be, it must be. |
4 participants
Context
Follow-up to #2: today the SDK's Node transport doesn't actually use HTTP/2.
EnvHttpProxyAgentwas constructed withoutallowH2, andkeepAliveTimeoutwas set to 1ms to keep CLIs exiting fast — so every Node request opened a fresh TCP+TLS connection.What was changed
EnvHttpProxyAgent({ allowH2: true })— ALPN-negotiated h2 with automatic h1.1 fallback.closeDefaultDispatcher()(package root) andCommsApi.close()so CLIs/scripts can drain the pool before exit.close().Benchmark (staging)
users.getSessionUser(first call, TLS handshake)workspaces.getWorkspace(reuses connection)threads.createThreadcomments.createCommentthreads.deleteThreadapi.close()Out of scope