Skip to content

feat(session): add CloseGraceful, closing a session without cutting off in-flight requests - #105

Open
burruplambert wants to merge 1 commit into
sardanioss:mainfrom
burruplambert:graceful-close
Open

feat(session): add CloseGraceful, closing a session without cutting off in-flight requests#105
burruplambert wants to merge 1 commit into
sardanioss:mainfrom
burruplambert:graceful-close

Conversation

@burruplambert

Copy link
Copy Markdown
Contributor

Problem

Session.Close tears the session's connections down at once. That is right for shutdown and wrong for rotation: a long-lived session that is being replaced usually still has a request or two on it, and Close interrupts them, so the reader sees use of closed network connection mid-body. Anyone rotating sessions today has to guess a grace period longer than their longest possible request and sleep it out before calling Close.

The primitive that avoids exactly this has existed since #83: persistentConn.requestClose / HTTP2Transport.retire defer a connection's close until its last in-flight response body is done. It is only ever applied to one evicted connection at a time. This PR applies it to the whole pool.

What it adds

Session.CloseGraceful() at every layer (root, session, transport.Transport, HTTP2Transport):

  • marks the session inactive, so new requests fail with ErrSessionClosed exactly as after Close;
  • takes every connection out of the HTTP/2 pool and retires it: idle ones close now, ones with a body still streaming close when release() fires at the end of that body;
  • returns immediately. Nothing waits on the drain.

Close() after CloseGraceful() is not a no-op: it hard-closes whatever is still draining (the http.Server.Shutdown / Close idiom), and it is safe to repeat in either order.

Close itself is unchanged in behaviour. Nothing on the wire changes. Nothing changes for anyone who does not call the new method.

Design notes

  • The cleanup loop is deliberately not stopped at CloseGraceful. It is the only thing that applies the abandoned-body bound (10 min without a byte) to retired connections, so stopping it would let a body that is never closed pin its socket for the process lifetime, which is the leak the retired list exists to prevent. The loop now exits on its own once the transport is closed and retired is empty; when CloseGraceful finds nothing to drain it stops the loop at once. Verified with a real 31s run that the goroutine exits after the drain.
  • HTTP2Transport.Close no longer early-returns on t.closed (that flag now also means "graceful close in progress"). Everything it touches is idempotent: stopCleanup is closed through a sync.Once, and the lists it drains are nil after a first call. Session tracks a draining flag for the same reason, since its Close guards on !active.
  • Because of that, Session.Close after CloseGraceful reaches Transport.Close a second time. Every sub-transport's Close was checked to be repeat-safe: HTTP/1.1 and HTTP/2 guard on their own state; http3.Transport.Close nils its lists; quic-go's Transport.close guards on closeErr and the UDP double-close error is discarded by closeWithTimeout; the udpbara tunnel and MASQUE conn guard on flags. A root-level test pins it in both orders, on auto and forced-H3 sessions.
  • HTTP/1.1 needs nothing: its transport only ever closes idle connections, and a checked-out one closes itself on return once the transport is closed, so Transport.CloseGraceful calls its plain Close. An e2e test pins that claim rather than leaving it to a code reading.
  • HTTP/3 has no per-request tracking at all in HTTP3Transport (no in-flight count, no body guard, no abandoned-body bound), so a real drain there is a separate change. For now CloseGraceful closes it as Close does, and the docs say so.
  • One incidental change: retire() now closes an idle connection off the caller's goroutine, like every other shutdown path in the file. Every existing caller already invoked it via go, so nothing observable changes for them, and it keeps CloseGraceful from stalling for up to 250ms per connection on an unresponsive peer's TLS close.
  • Known and inherent: a request that passed the closed check but had not yet reserved its connection (a few instructions wide) can still get http2: transport closed. That is identical to Close and to any check-then-use boundary; it surfaces as an error where errors are already expected.

Tests

  • transport/close_graceful_test.go: fake-connection unit tests in the style of the DoStream: pool cleanup closes an active HTTP/2 response body after ~120 seconds #83 tests. Idle closes now; streaming defers and new requests are refused; an abandoned body is still reclaimed after a graceful close; Close after graceful forces; both orders idempotent.
  • transport/close_graceful_e2e_test.go: against real in-process servers. An HTTP/2 body mid-stream survives CloseGraceful and is delivered in full, new requests are refused, and the connection closes when the body ends; the same scenario under Close is pinned for contrast (reader is cut off); a plain Do in flight completes; the HTTP/1.1 stream case; a dial-vs-CloseGraceful race hammer publishes nothing into the closed pool.
  • close_graceful_test.go (root, whitelisted in .gitignore like the other root-level regression locks): the public Session API via DoStream, and CloseGraceful / Close in both orders on auto and forced-H3 sessions.

All of transport, session, pool, root and client (minus the two tests that hit tls.peet.ws, whose certificate has expired) pass under -race; the new tests were run 25x under -race.

Docs

CHANGELOG.md entry under Unreleased; CloseGraceful listed under Lifecycle on the Go bindings page; the observability page's IsActive statements now name both closes; one bullet in the long-running-scraper recipe's "Things to NOT do".

Follow-ups (not in this PR)

  • C API / bindings: httpcloak_session_close_graceful mirroring httpcloak_session_free (bindings/clib is a separate module pinned to a released version, so it has to follow a release), then the Python / Node / .NET wrappers.
  • The lower-level Client / pool.HostPool stack has the same requestClose primitive and could get the same method.
  • A real HTTP/3 drain, once HTTP3Transport has in-flight tracking.

…ff in-flight requests

Session.Close tears the session's connections down at once. That is right for
shutdown and wrong for rotation: a long-lived session that is being replaced
usually still has a request or two on it, and Close interrupts them, so the
reader sees "use of closed network connection" mid-body. Anyone rotating
sessions today has to guess a grace period longer than their longest possible
request and sleep it out before calling Close.

The primitive that avoids exactly this already exists since sardanioss#83:
persistentConn.requestClose / HTTP2Transport.retire defer a connection's close
until its last in-flight response body is done. It is only ever applied to one
evicted connection at a time. This applies it to the whole pool.

Session.CloseGraceful (root, session, transport.Transport, HTTP2Transport):

  - marks the session inactive, so new requests fail with ErrSessionClosed
    exactly as after Close;
  - takes every connection out of the HTTP/2 pool and retires it: idle ones
    close now, ones with a body still streaming close when release() fires
    at the end of that body;
  - returns immediately. Nothing waits on the drain.

The cleanup loop is deliberately NOT stopped at CloseGraceful. It is the only
thing that applies the abandoned-body bound (10 min without a byte) to retired
connections, so stopping it would let a body that is never closed pin its
socket for the process lifetime, which is the leak the retired list exists to
prevent. The loop now exits on its own once the transport is closed and the
retired list is empty; when CloseGraceful finds nothing to drain it stops the
loop at once.

Close after CloseGraceful is not a no-op: it hard-closes whatever is still
draining, and it is safe to repeat. To make that work, HTTP2Transport.Close no
longer early-returns on t.closed (it was the only thing that flag guarded, and
the lists it drains are nil after a first call anyway) and the stopCleanup
channel is closed through a sync.Once. Session tracks a draining flag for the
same reason, since Close guards on !active. That does mean Session.Close now
reaches Transport.Close a second time after a CloseGraceful; every
sub-transport's Close was checked to be repeat-safe (HTTP/1.1 and HTTP/2 guard
on their own state, quic-go's Transport.close guards on closeErr and the UDP
double-close error is discarded, the udpbara tunnel and MASQUE conn guard on
flags) and a root-level test pins it in both orders, on auto and forced-H3
sessions.

HTTP/1.1 needs nothing: its transport only ever closes idle connections, and a
checked-out one closes itself on return once the transport is closed, so
Transport.CloseGraceful calls its plain Close; an e2e test pins that claim
rather than leaving it to a code reading. HTTP/3 has no per-request tracking
at all in HTTP3Transport (no in-flight count, no body guard, no abandoned-body
bound), so a real drain there is a separate change; for now CloseGraceful
closes it as Close does, and the docs say so.

One incidental change: retire() now closes an idle connection off the caller's
goroutine, like every other shutdown path in the file. Every existing caller
already invoked it via go, so nothing observable changes for them, and it keeps
CloseGraceful from stalling for up to 250ms per connection on an unresponsive
peer's TLS close.

Close itself is unchanged in behaviour. Nothing on the wire changes.

Tests:
  transport/close_graceful_test.go     fake-connection unit tests in the style
                                       of the sardanioss#83 tests: idle closes now,
                                       streaming defers, abandoned body is still
                                       reclaimed, Close after graceful forces,
                                       both orders idempotent
  transport/close_graceful_e2e_test.go against real servers: an HTTP/2 body
                                       mid-stream survives CloseGraceful and is
                                       delivered in full, new requests refused,
                                       connection closes when the body ends; the
                                       same scenario under Close is pinned for
                                       contrast (reader is cut off); Do in
                                       flight completes; the HTTP/1.1 stream
                                       case; a dial/CloseGraceful race hammer
                                       publishes nothing into the closed pool
  close_graceful_test.go               the public Session API via DoStream, and
                                       CloseGraceful/Close in both orders on
                                       auto and forced-H3 sessions; whitelisted
                                       in .gitignore like the other root-level
                                       regression locks

Docs: CloseGraceful listed under Lifecycle on the Go bindings page, and the
observability page's IsActive statements now name both closes.
@vercel

vercel Bot commented Aug 18, 2026

Copy link
Copy Markdown

@burruplambert is attempting to deploy a commit to the sardanioss' projects Team on Vercel.

A member of the Team first needs to authorize it.

@sardanioss

Copy link
Copy Markdown
Owner

Good idea. browser also drains when it retires a session and only cuts streams when something's actually gone wrong, though most of the actual users just close tabs or the chrome itself that was my reason for the abrupt closing rather than draining. Though my hesitation is the shape. A second method won't be free, it's another C export plus three wrappers plus docs in every binding. A session option like WithGracefulClose() keeps one Close everywhere instead. Not saying that's better, just want it considered.

What would settle it for me is that does anyone actually need a hard close sometimes and a drain other times on the same session? if yes then the separate method wins and I'll drop it.

Transport side looks solid btw.

@burruplambert

Copy link
Copy Markdown
Contributor Author

For me the answer is no. I'd just use WithGracefulClose(). I made this PR because I am doing high-throughput scraping and I have a site where I need to rotate TLS sessions every X requests otherwise rate limiting occurs. I don't want to drop in-flight requests so on my end I have to keep track of the sessionTimeout passed to WithSessionTimeout() then do sleep + manual close. E.g:

const sessionTimeout = 30 * time.Second

// Session.Close tears down connections immediately, so a replaced session
// has to outlive the longest request that could still be in flight on it.
const retireGrace = sessionTimeout + 5*time.Second

sess, _ := httpcloak.NewSession(
    httpcloak.WithPreset("chrome-146"),
    httpcloak.WithSessionTimeout(sessionTimeout),
    ...
)

// ... after N requests, swap in a fresh session and retire the old one:
retire := func(old *httpcloak.Session) {
    time.Sleep(retireGrace) // guess a bound on in-flight requests
    old.Close()
}
go retire(sess)

WithGracefulClose() would look something like:

sess, _ := httpcloak.NewSession(
    httpcloak.WithPreset("chrome-146"),
    httpcloak.WithSessionTimeout(sessionTimeout),
    httpcloak.WithGracefulClose(),
    ...
)

// ... after N requests:
sess.Close() // returns now; in-flight bodies finish, then their conns go

I checked bogdanfinn/tls-client and Noooste/azuretls-client. Neither has a hard close: tls-client's only close verb is CloseIdleConnections() (there is no Close()), and azuretls' Session.Close() calls CloseIdleConnections() on H1/H2 (H3 is closed hard). Both close idle connections and let in-flight requests finish. httpcloak has no non-destructive close at all today. Close() and Refresh() both cut busy connections. The mechanism differs (httpcloak's H2 pool has no close-idle primitive, so the PR tracks in-flight conns explicitly), but the semantics match theirs.

There's no naming baseline (tls-client calls it CloseIdleConnections(), azuretls calls it Close()). However both inherit the stdlib Transport.CloseIdleConnections semantics: close idle, let in-flight finish. It's the only close either offers. If we do WithGracefulClose() it lets a session match that opt-in without changing what Close() means for anyone else.

Let me know and I'll rework the PR as WithGracefulClose() and leave the transport side as is. I'll state on the option's doc: Close() still makes the session unusable and takes no new requests in both modes; the option only changes what happens to requests already in flight (they finish, then their connections close, bounded by the abandoned-body timeout) instead of being cut off.

@sardanioss

Copy link
Copy Markdown
Owner

Got it, I'll take over from here, if there are any changes to make I'll do so, just wanted to know a bit more about the situation, thanks for the clarification. As for the abrupt close, I guess I'll keep it default with graceful shutdown as the second method(don't want any surprises for people who've already gotten accustomed to the old behaviour), I'll add FFI bindings first for this before releasing it.

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.

2 participants