ConnectionFilter has no end-of-connection hook, so downstream connections can be counted starting but never ending
What is the problem your feature solves, or the need it fulfills?
A process that uses Pingora as its entry point cannot report how many downstream connections it currently has. A connection starting can be observed today; a connection ending cannot be observed at all.
ConnectionFilter::should_accept (added by #671, behind the connection_filter feature) is
called after accept() and before the TLS handshake, which is the right moment to see a
connection start. Nothing is called when one ends. run_endpoint spawns one task per
connection, and that task has three exits — handshake timeout, handshake error, and
handle_event returning — and none of them notifies anything. So a number built on
should_accept can only ever go up.
This has been asked for more than once:
- #118, "on connect phase for incoming connections" — closed as completed by #671, which covers the accept moment only.
- #295 — asked specifically for the number of active connections (increment on connect,
decrement on disconnect). The suggested answer was a userland
Dropguard; the issue was closed by the stale bot, the reporter's last comment being "still relevant". - #337 — asked how to track client connect/disconnect (for WebSocket); the answer was to
use
Tracer, and the issue was closed as completed.
Pingora already has exactly this shape, on the other side: upstreams::peer::Tracing
provides on_connected / on_disconnected, and Tracer is the answer people are pointed
at on #245, #295 and #337. But a Tracer is a field on PeerOptions and counts connections
to upstreams; there is no equivalent for connections from downstream. The alternatives
section below says why that is a different question rather than a smaller one.
Worth noting from the other side: pingora-prometheus and pingora-foundations already give
a place to publish such a number and an endpoint to serve it from. What is missing is anything
in core that reports it.
One caveat we are not asking you to change, mentioned so a count is not oversold: in
ListenerEndpoint::accept(), if the stream has no socket digest or the digest has no peer
address, should_accept is not called at all and the connection is accepted by default. For a
filter that is a safe default; it does mean a count of starts is not exhaustive.
Describe the solution you'd like
Extend the existing ConnectionFilter seam rather than introduce a second one. It already
carries the plumbing (a per-endpoint field, a builder method, Listeners::set_connection_filter)
and is already gated behind connection_filter, which is off by default.
One addition: a defaulted fn connection_closed(&self), called once for each connection
should_accept was consulted about, when that connection is gone. No signature changes, no
new data crossing the boundary, and nothing for existing implementors to do.
The pairing is the part worth pinning down: as noted above, should_accept is skipped
entirely when there is no peer address. A close hook that fired for every accepted
connection would therefore not pair with it, and the obvious +1 / -1 implementation would
underflow. Firing it exactly where should_accept ran keeps the two in step.
#[async_trait]
impl ConnectionFilter for MyConnectionCount {
async fn should_accept(&self, _addr: Option<&SocketAddr>) -> bool {
self.live.fetch_add(1, Relaxed);
true
}
// new
fn connection_closed(&self) { self.live.fetch_sub(1, Relaxed); }
}On attributing connections to a particular listener: we are deliberately not asking for
that here. Today Listeners::set_connection_filter installs one instance across every
endpoint (and add_endpoint clones that same one), so the hook above yields a process-wide
live count — which is more than is obtainable now. Per-listener attribution follows from #941
by construction, since a filter instance attached to one address already knows its address.
We would rather wait for that than ask for a second mechanism.
Two things we learned the hard way carrying this in a fork, offered for what they are worth rather than as a prescription:
- The end hook wants to be synchronous. The only way we found to guarantee it fires is a
value owned by the spawned task that calls the hook from
Drop— andDropcannot await. Anasyncclose hook therefore forces either aspawnper closing connection or a blocking bridge, both worse than a plainfn.should_acceptcan stayasync. - Drive it from that guard, not from explicit calls at each exit. The task has three exits; an explicit call has to be written three times, and the failure mode of missing one is silent — a live-connection number that never comes back down, sitting next to a total that looks perfectly healthy.
Backwards compatibility: a defaulted method is source-compatible for existing implementors, and the feature is opt-in, so nothing changes for anyone who does not implement it.
One thing worth knowing before shaping this: the trait exists twice. The real one is in
listeners/connection_filter.rs under the feature; listeners/mod.rs defines a stub for
when the feature is off, described as being there "for API compatibility". Their signatures
already differ — the stub's should_accept is synchronous and takes &SocketAddr, the real
one is async and takes Option<&SocketAddr> — so any addition here needs a decision about
the stub too. We mention it only because it shapes the change, not as a separate report.
On what this would add to pingora-core: we are aware that core deliberately no longer
carries metrics. #560 and #822 asked for prometheus to be made optional, and 842ddd9
answered by moving the Prometheus HTTP app out into its own pingora-prometheus crate,
which now builds entirely on core's public API. What we are asking for sits on the other
side of that same line: one notification, with no metrics dependency, no counter/gauge
distinction and no label vocabulary. The counting stays in the caller, exactly as
pingora-prometheus now sits outside core.
A scope question we would rather ask than assume: connection_filter is named for
filtering, and observing when a connection ends is a different concern. If you would prefer
it to live under a differently named feature, or on a sibling trait, we are happy to shape a
PR that way. What we would like to avoid is a second, parallel wiring path alongside the one
ConnectionFilter already has.
Describe alternatives you've considered
upstreams::peer::Tracer— the answer given on #245, #295 and #337, so it is worth addressing up front rather than after a round trip. ATraceris placed inPeerOptionsand itson_connected/on_disconnectedfire for connections Pingora opens to an upstream, counting active plus pooled ones. That is a different population from the connections a listener is currently holding: a downstream connection that never opens an upstream connection produces no tracer events at all — one rejected by a filter, one that fails or times out in the TLS handshake, a request served from cache or by a local handler, or any L4 application that never dials out. Nothing inlisteners/orservices/listening.rsreferences it.ServerApp::cleanup/HttpServerApp::http_cleanup— suggested on #118, so worth ruling out explicitly. These are per-service, not per-connection: the doc comment oncleanupsays it is "called once after the service stops listening to its endpoints", and the call site inrun_endpointis after the accept loop has exited. They cannot see individual connections at all.- Counting in userland with a
Dropguard (also suggested on #295). It cannot cover the window this is about: connections that time out or fail duringio.handshake()never reach the application, becausehandle_eventis only called on the successful branch. For an entry point those are a population you specifically want to see. - Polling the operating system (
netstat-style, also considered on #295): it is a sampling answer to a question about state transitions, and attributing sockets back to a particular listener of a particular process is awkward at best. - A separate observer trait alongside
ConnectionFilter: this duplicates wiring that already exists (per-endpoint field, builder method,Listeners::set_*), which is why we are proposing to extend the existing seam. See the scope question above if you disagree. - Carrying it in a fork — what we do today. It works, but it is a permanent rebase cost for something that looks generally useful rather than specific to us, which is why we are asking here instead of keeping it.
Additional context
- Related, on the seam itself: #671 (the
ConnectionFiltertrait), #118, #295, #337. - Related, on listener identity, which we are deliberately leaving alone: #941 (open) attaches filters per address, which is how we would expect per-listener attribution to arrive; #988, with PR #991 reporting the addresses a service actually bound and PR #990 fixing the fd-table collision it needs (all open).
- Related, on end-of-something hooks: #751 (open since 2025-11) adds
finish_downstream_sessiontoProxyHttp. That is a per-session hook inpingora-proxy; what is asked for above is a per-connection notification inpingora-core, so it also covers L4 applications and connections that never complete a handshake. - We maintain a fork carrying this capability and would be glad to send the PR, in whatever shape you prefer.
Pingora version: main @ 09696b5
Source: cloudflare/pingora