Repository navigation
Conversation
e7cb05e to
7443f35
Compare
|
Joe to do: write a script to send a bunch of concurrent requests and see how the queue behaves |
josephjclark
left a comment
josephjclark
left a comment
There was a problem hiding this comment.
Just some initial thoughts, shooting from the hip
| // FIFO semaphore capping how many python processes bridge.ts runs at once. | ||
| // Requests past the cap wait here instead of spawning. | ||
|
|
||
| const DEFAULT_MAX_CONCURRENT = 10; |
There was a problem hiding this comment.
IS 10 a random number?
Do we have any idea of the memory overhead per fork?
There was a problem hiding this comment.
yeah 10 is a random number. yet to be validated
| : DEFAULT_MAX_CONCURRENT; | ||
| }; | ||
|
|
||
| let active = 0; |
There was a problem hiding this comment.
So this is module scoped. Only one queue per process.
Is that bad for testing? For re-use? Should there be a class or at least some instance based here?
Open question, maybe this is fine. Just sticks out at me a little as a weak architecture
| let active = 0; | ||
| const queue: (() => void)[] = []; | ||
|
|
||
| export const activeCount = (): number => active; |
There was a problem hiding this comment.
Are either of these actually neccessary? You could just export active and queue?
Looks like they're both just used by unit tests anyway
There was a problem hiding this comment.
used by the test only.
|
|
||
| /** Releases a slot acquireSlot() granted, straight to the next waiter. Call | ||
| * exactly once per resolved acquireSlot(). */ | ||
| export const releaseSlot = (): void => { |
There was a problem hiding this comment.
Should this be guarded to be a bit more idempotent? Or are we absolutely sure it's super tight?
Or what if every slot has its own release functuon, like with the cancel function?
josephjclark
left a comment
josephjclark
left a comment
There was a problem hiding this comment.
Oh and we mentioned raising a sentry alert whenever an item is added to the queue? Or perhaps if more than N items are added
| let started = false; | ||
| const { promise, cancel } = enqueue(scriptName, () => { | ||
| started = true; | ||
| return runFn(scriptName, port, inputPath, outputPath, onLog, onEvent, signal); |
There was a problem hiding this comment.
This is nice - much cleaner control logic I'd say?
josephjclark
left a comment
josephjclark
left a comment
There was a problem hiding this comment.
Looking good. I'd still prefer to see the queue instanced but I'm happy to accept this.
I'll run some manual tests tomorrow morning.
2 participants
Short Description
Caps how many python child processes
bridge.tsruns at once(
APOLLO_MAX_CONCURRENT_PYTHON, default 10). Requests past the cap queue inFIFO order in Node instead of spawning immediately, so a burst of traffic
can't pile an unbounded number of python interpreters on top of each other.
Fixes #
Implementation Details
Today every request to a python service spawns (or forks, if
APOLLO_FORK_SERVERis on) a process immediately, with no limit on how manyrun at once. A burst of concurrent requests has no backpressure before it
hits the host's CPU/memory limits.
platform/src/util/python-queue.ts(new) — a small hand-rolled FIFOsemaphore:
acquireSlot(scriptName, signal)grants immediately while underAPOLLO_MAX_CONCURRENT_PYTHON, otherwise queues the caller. Ifsignalaborts while still queued (the client went away before its turn), the wait
rejects and the slot is never consumed - the request never spawns a process
at all.
releaseSlot()hands a freed slot straight to the next waiter.platform/src/bridge.ts-run()now callsacquireSlot()rightbefore doing any real work, and releases exactly once (guarded against
double-release) from whichever path ends the run first. This applies
uniformly to both execution paths: forking from the warm master
(
USE_FORK_SERVER) and spawning a fresh interpreter - either way it's onepython process doing a service's worth of real work, so both are capped the
same way.
.env.example- documentsAPOLLO_MAX_CONCURRENT_PYTHON(default 10).Testing:
platform/test/util/python-queue.test.ts- pure unit tests of the queuelogic: immediate grant under the cap, FIFO ordering past it, a queued wait
rejecting on abort without consuming a slot, and an already-aborted signal
rejecting immediately when the cap is full.
platform/test/bridge-queue.test.ts- real-subprocess integration tests viathe existing
_cancel_probefixture: tworun()calls serialize under acap of 1, and a call cancelled while queued never spawns a process.
bun dev: underAPOLLO_MAX_CONCURRENT_PYTHON=1/2,concurrent requests to
/services/echocorrectly queue(
queueing echo: 1/1 python processes running, 1 waiting) and completeserially; repeated with
APOLLO_FORK_SERVER=trueto confirm the samequeueing holds for the fork path.
bun test platform/test: 122 pass, 7 fail (pre-existing, unrelated -live-Postgres tests with no DB available locally).
AI Usage
Please disclose whether you've used AI in this work (it's cool, we just want to
know!):
You can read more details in our
Responsible AI Policy