feat(session): add CloseGraceful, closing a session without cutting off in-flight requests - #105
feat(session): add CloseGraceful, closing a session without cutting off in-flight requests#105burruplambert wants to merge 1 commit into
Conversation
…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.
|
@burruplambert is attempting to deploy a commit to the sardanioss' projects Team on Vercel. A member of the Team first needs to authorize it. |
|
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. |
|
For me the answer is no. I'd just use
I checked bogdanfinn/tls-client and Noooste/azuretls-client. Neither has a hard close: tls-client's only close verb is There's no naming baseline (tls-client calls it Let me know and I'll rework the PR as |
|
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. |
Problem
Session.Closetears 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, andCloseinterrupts them, so the reader seesuse of closed network connectionmid-body. Anyone rotating sessions today has to guess a grace period longer than their longest possible request and sleep it out before callingClose.The primitive that avoids exactly this has existed since #83:
persistentConn.requestClose/HTTP2Transport.retiredefer 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):ErrSessionClosedexactly as afterClose;release()fires at the end of that body;Close()afterCloseGraceful()is not a no-op: it hard-closes whatever is still draining (thehttp.Server.Shutdown/Closeidiom), and it is safe to repeat in either order.Closeitself is unchanged in behaviour. Nothing on the wire changes. Nothing changes for anyone who does not call the new method.Design notes
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 theretiredlist exists to prevent. The loop now exits on its own once the transport is closed andretiredis empty; whenCloseGracefulfinds nothing to drain it stops the loop at once. Verified with a real 31s run that the goroutine exits after the drain.HTTP2Transport.Closeno longer early-returns ont.closed(that flag now also means "graceful close in progress"). Everything it touches is idempotent:stopCleanupis closed through async.Once, and the lists it drains are nil after a first call.Sessiontracks adrainingflag for the same reason, since itsCloseguards on!active.Session.CloseafterCloseGracefulreachesTransport.Closea second time. Every sub-transport'sClosewas checked to be repeat-safe: HTTP/1.1 and HTTP/2 guard on their own state;http3.Transport.Closenils its lists; quic-go'sTransport.closeguards oncloseErrand the UDP double-close error is discarded bycloseWithTimeout; the udpbara tunnel and MASQUE conn guard on flags. A root-level test pins it in both orders, on auto and forced-H3 sessions.Transport.CloseGracefulcalls its plainClose. An e2e test pins that claim rather than leaving it to a code reading.HTTP3Transport(no in-flight count, no body guard, no abandoned-body bound), so a real drain there is a separate change. For nowCloseGracefulcloses it asClosedoes, and the docs say so.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 viago, so nothing observable changes for them, and it keepsCloseGracefulfrom stalling for up to 250ms per connection on an unresponsive peer's TLS close.http2: transport closed. That is identical toCloseand 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;Closeafter graceful forces; both orders idempotent.transport/close_graceful_e2e_test.go: against real in-process servers. An HTTP/2 body mid-stream survivesCloseGracefuland is delivered in full, new requests are refused, and the connection closes when the body ends; the same scenario underCloseis pinned for contrast (reader is cut off); a plainDoin flight completes; the HTTP/1.1 stream case; a dial-vs-CloseGracefulrace hammer publishes nothing into the closed pool.close_graceful_test.go(root, whitelisted in.gitignorelike the other root-level regression locks): the publicSessionAPI viaDoStream, andCloseGraceful/Closein both orders on auto and forced-H3 sessions.All of
transport,session,pool, root andclient(minus the two tests that hittls.peet.ws, whose certificate has expired) pass under-race; the new tests were run 25x under-race.Docs
CHANGELOG.mdentry under Unreleased;CloseGracefullisted under Lifecycle on the Go bindings page; the observability page'sIsActivestatements now name both closes; one bullet in the long-running-scraper recipe's "Things to NOT do".Follow-ups (not in this PR)
httpcloak_session_close_gracefulmirroringhttpcloak_session_free(bindings/clibis a separate module pinned to a released version, so it has to follow a release), then the Python / Node / .NET wrappers.Client/pool.HostPoolstack has the samerequestCloseprimitive and could get the same method.HTTP3Transporthas in-flight tracking.