proxy: add local connection limit to ListenConnections #269

enirox001 commented at 10:00 AM on April 8, 2026: contributor

This adds an optional local connection limit toListenConnections().

Previously, ListenConnections() would accept incoming connections indefinitely. This branch adds an optional max_connections parameter so a listener can stop accepting new connections once a per-listener cap is reached, and resume accepting when an existing connection disconnects.

The limit is local to the listener instead of global to the EventLoop. This keeps the state and behavior scoped to the listening socket, and is closer to the direction discussed downstream for per--ipcbind limits.

This also adds a test covering the behavior with max_connections=1, verifying that:

the first client is accepted normally
a second client is not accepted while the first remains connected
the second client is accepted after the first disconnects

This is a draft follow-up to the IPC FD reservation work

DrahtBot commented at 10:01 AM on April 8, 2026: none

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Approach ACK	ryanofsky

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#231 (Add windows support by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

enirox001 marked this as a draft on Apr 8, 2026

enirox001 force-pushed on Apr 8, 2026

in include/mp/proxy-io.h:829 in 84ed607b39

 821 | @@ -820,8 +822,19 @@ std::unique_ptr<ProxyClient<InitInterface>> ConnectStream(EventLoop& loop, int f
 822 |  //! handles requests from the stream by calling the init object. Embed the
 823 |  //! ProxyServer in a Connection object that is stored and erased if
 824 |  //! disconnected. This should be called from the event loop thread.
 825 | +template <typename InitInterface, typename InitImpl, typename OnDisconnect>
 826 | +void _Serve(EventLoop& loop, kj::Own<kj::AsyncIoStream>&& stream, InitImpl& init, OnDisconnect&& on_disconnect);
 827 | +
 828 |  template <typename InitInterface, typename InitImpl>
 829 |  void _Serve(EventLoop& loop, kj::Own<kj::AsyncIoStream>&& stream, InitImpl& init)

ryanofsky commented at 2:02 PM on April 8, 2026:

In commit "proxy: add local connection limit to ListenConnections" (84ed607b390e24dd0d41cd95d0159bfeadfaf5d8)

Since _Serve is an internal function only called in a few places, I think it would be less confusing if it was not overloaded, and just always required an on_disconnect parameter, even if it requires a little extra verbosity at some call sites.

enirox001 commented at 9:58 AM on April 9, 2026:

Done. Simplified this now so _Serve always takes an on_disconnect callback instead of overloading it. Since it is only used internally

in include/mp/proxy-io.h:865 in 84ed607b39 outdated

 861 | +        : listener(kj::mv(listener_)), max_connections(max_connections_) {}
 862 | +
 863 | +    kj::Own<kj::ConnectionReceiver> listener;
 864 | +    std::optional<size_t> max_connections;
 865 | +    size_t active_connections{0};
 866 | +    bool accept_pending{false};

ryanofsky commented at 2:08 PM on April 8, 2026:

In commit "proxy: add local connection limit to ListenConnections" (84ed607b390e24dd0d41cd95d0159bfeadfaf5d8)

Curious if this accept_pending variable is actually necessary or if code could just compare active_connections and max_connections when deciding whether to listen. Would prefer to avoid redundancy in the state representation if possible even if makes individual checks more a little more verbose.

If accept_pending really is necessary would be a good to have a short comment about why.

enirox001 commented at 9:59 AM on April 9, 2026:

I kept accept_pending, but added a short comment explaining why it is needed here. active_connections only counts accepted connections, so without a separate flag nested _Listen() calls could post multiple pending accept() calls before active_connections is incremented.

in test/mp/test/test.cpp:51 in 84ed607b39

  46 |  namespace mp {
  47 |  namespace test {
  48 |  
  49 | +namespace {
  50 | +
  51 | +class UnixListener

ryanofsky commented at 2:43 PM on April 8, 2026:

In commit "proxy: add local connection limit to ListenConnections" (84ed607b390e24dd0d41cd95d0159bfeadfaf5d8)

Would be good to introduce the test in a separate commit and maybe separate file if it doesn't share much code with existing test.

IMO would be nice if it test was introduced in an intial commit then updated after connection limits are added so it's easier to see how connection limits are tested separately from connection setup.

enirox001 commented at 10:02 AM on April 9, 2026:

Split the listener coverage out into a dedicated listen_tests.cpp file and reordered the history so the baseline ListenConnections() test is introduced first, then extended in follow-up commit with the local connection limit coverage.

ryanofsky commented at 2:44 PM on April 8, 2026: collaborator

Approach ACK 84ed607b390e24dd0d41cd95d0159bfeadfaf5d8. Implementation of local connection limit here looks almost exactly like I would have expected.

I do think it would be helpful to see a draft PR in the bitcoin repo using this API (it should be fine to make libmultiprocess changes there and let the lint CI job fail so you don't need to mess with subtrees) because the approach in https://github.com/bitcoin/bitcoin/pull/34978 of adding a global connection limit option isn't exactly compatible with the implementation here of implementing a per-address connection limit.

I'd personally prefer using per-address limits over introducing a global limit but both approaches seem reasonable

test: add dedicated ListenConnections coverage

Add a separate listen_tests.cpp file with reusable UnixListener, ClientSetup, and ListenSetup helpers for exercising ListenConnections() with real Unix domain sockets.

The new test covers the baseline behavior that ListenConnections() accepts an incoming connection and serves requests over it. Keeping this coverage separate from the existing general proxy tests makes the socket listener setup easier to review and provides a clearer place to extend listener-specific behavior in follow-up commits.

8c47a3aba8

enirox001 force-pushed on Apr 9, 2026

proxy: add local connection limit to ListenConnections

Add an optional max_connections parameter to ListenConnections() so a listener can stop accepting new connections after reaching a local connection cap and resume accepting after an existing connection disconnects.

Implement the limit with listener-local state tracking the listening socket, maximum number of active connections, and whether an async accept() has already been posted. This keeps the limit scoped to the individual listener instead of introducing global EventLoop state.

Extend the dedicated listener test coverage to verify that with max_connections=1 the first client is accepted normally, a second client is not accepted while the first remains connected, and the second client is accepted after the first disconnects.

8511c68f8b

enirox001 force-pushed on Apr 9, 2026

enirox001 commented at 10:29 AM on April 9, 2026: contributor

Thanks for the review @ryanofsky .

I addressed the cleanup points in the latest push:

_Serve is now explicit at all call sites
accept_pending is documented,
listener coverage now lives in a dedicated listen_tests.cpp file introduced in a separate commit before the local-limit extension.

I’m also planning to put together a draft Bitcoin Core PR using this API so the per-address approach can be evaluated downstream against the current global-limit direction.

enirox001 commented at 1:48 PM on April 9, 2026: contributor

I put together the downstream draft using this API here bitcoin/bitcoin#35037

It uses per--ipcbind max-connections=<n> options, threads the parsed per-address limit into ListenConnections(), and adds downstream coverage for the local-limit behavior.