[pull] master from cube-js:master - #694
Merged
Merged
Conversation
* feat(cubestore): cap concurrent websocket connections per user
Adds CUBESTORE_MAX_WS_CONNECTIONS_PER_USER, an optional per-node cap on how
many concurrent WebSocket connections one authenticated user may hold. A
client that opens connections faster than it closes them can otherwise consume
the whole process file descriptor table, which affects every other user of
that node.
At the limit the user's oldest connection is closed to admit the new one,
rather than the new one being refused: a client that still needs the closed
connection reconnects, while one that had forgotten about it simply loses it.
It is closed with a close frame, so the client sees a clean close and can
reconnect on its own terms.
A slot is taken at the websocket upgrade and released by a guard held for the
lifetime of the connection task, so it comes back on any exit path: a close
frame, a transport error, a panic, a cancelled task, or a connection that dies
before the upgrade completes. Live connections are keyed by a monotonic id, so
an evicting admission and the victim's own later drop cannot remove each
other's entry, and the limit never overshoots.
The limit defaults to 0, which disables the cap and leaves behaviour unchanged.
* fix(cubestore): bound the evicted connection's close and name its reason
Sending the close frame flushes, so a peer whose receive window is closed would
park the connection task indefinitely with the descriptor still held. That
matters more than a stray task: the entry leaves the counter when the
connection is evicted rather than when its task ends, so the cap would read as
satisfied while the process held more connections than it allows. The send is
now bounded, and the bound is deliberately short — the steady-state overshoot
is the eviction rate times this timeout, and a close frame is a handful of
bytes, so a peer that cannot take them in two seconds is not going to.
The frame now carries close code 1013 ("Try Again Later") and a reason, so a
client can tell being recycled from a transport failure and reconnect rather
than report an error.
Adds an end-to-end test: with a cap of one, a second connection for the same
authenticated user evicts the first, which receives that close code and reason,
while a different user connects unaffected. It covers the parts the unit tests
cannot reach — the select branch, the close path and the guard living in the
connection task. The read is bounded, so a regression in the eviction path
fails the test rather than hanging it.
* refactor(cubestore): bound every take of the connection counter's lock
The counter's critical section is a map lookup and an insert with nothing
awaited inside it — `acquire` is not an `async fn`, so nothing can be — which
is the shape the `tokio::sync::Mutex` docs point at a standard-library lock
for. That makes a stall unreachable by reasoning about the callers; taking the
lock with a bound makes it unreachable without needing that reasoning at all.
Switches to parking_lot for `try_lock_for`. It also removes poisoning, which
had to be recovered by hand before: there is nothing meaningful to recover for
a map of live connections, and a poisoned unwrap in the destructor would have
aborted the process. parking_lot is already in the tree at this version, so the
lockfile gains one dependency edge and no crate.
Both callers fail open, because the cap is a safety net rather than a
correctness invariant: an unavailable counter admits the connection untracked
instead of refusing or stalling it, and a finished connection leaves its entry
behind to be evicted — that entry is the oldest one of its user by
construction, and cancelling a finished connection's token is a no-op. Both
paths log, so a bound that is ever reached is visible instead of quietly
weakening the cap.
The bound is 100ms rather than something tighter because a thread holding the
lock can be descheduled for milliseconds under CPU pressure, and giving up then
would stop counting connections exactly when the cap matters most.
* refactor(cubestore): give the connection counter's lock a second to be taken
The bound exists to make a stall impossible without reasoning about the
callers, not to be tight, and the failure it guards against is one-sided: a
thread holding the lock can be descheduled for a long time under CPU pressure,
and giving up then would quietly stop counting connections exactly when the cap
matters most. A second is far past anything a map lookup and an insert can
take, and giving up is logged either way.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )