Simplify the worker and reorganize the execution pools - #1
Conversation
Nothing references it anymore, inside or outside the gem: it was internal API, always instantiated by the worker and never something apps would configure or subclass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Keeping the inflight gauge fresh on every claim and job completion required a mutex and dirty-tracking in the worker, an extra metadata update per claiming poll (up to 10 per second per worker at the default polling interval), and had the poller and heartbeat threads racing to update the same row. The gauge doesn't need that freshness: it's a monitoring convenience, the exact count is always available from claimed executions, and the heartbeat already refreshes it with bounded staleness. With the dirty-tracking gone, the pool callback's only job is waking the worker when capacity frees up, so it also gets its old name back: on_idle. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The configuration already validates that workers specify either threads or fibers but not both, and that's where option validation belongs; the worker doesn't validate any of its other options. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
58fd807 to
29a1b25
Compare
In the default fork supervisor mode, workers are instantiated by the supervisor before forking, so a reactor thread started when the pool is built wouldn't survive the fork. Starting it when the first execution is posted makes the fiber pool safe to build at any point, matching the thread pool, which already builds its executor lazily on first use. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Build the execution pool directly when the worker is initialized, which is safe now that both pools start their threads lazily. Merge the worker defaults unconditionally and choose the pool type with a single check on the fibers option, relying on the configuration's validation to prevent threads and fibers from being combined. If a worker is instantiated directly with no pool options, it defaults to a thread pool; if it somehow gets both options, fibers win. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ebb8e18 to
6608b6e
Compare
29a1b25 to
ebb8e18
Compare
The in-flight gauge was refreshed on heartbeats only, so it could show counts that were stale by up to a heartbeat interval; better not to show it than to show stale information, especially given the exact count is always available from claimed executions. The pool type and size are constant, and the pool knows both, so the worker can store them directly when registering, as it did before, and the pools don't need to provide changing metadata at all. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The rescue inside the future covers job execution, but an error raised when restoring capacity or waking up the worker would escape it and reject the future with nobody watching. Attach the on_rejection callback the old pool had as a backstop, so those errors are also reported via handle_thread_error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aba14ce to
d66bb3f
Compare
Instead of tracking whether capacity had been reserved with a local flag, scope the rescue to the code that runs after the reservation, which is the only part that needs to restore it on failure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ff25296 to
e6ed969
Compare
Both pools share the capacity ledger: the reserve/restore accounting, the idle notification, and the reserve-then-schedule shape of post. Move all of that to ExecutionPools::Base, leaving each pool with just its scheduling mechanism: the thread pool schedules executions on a Concurrent::ThreadPoolExecutor via schedule, and the fiber pool hands them to its reactor, adding its fatal-error and shutdown guards on top of the base's post. The default way of performing an execution, wrapped in the app executor, reporting errors and restoring capacity, also lives in the base class; the fiber pool overrides it to treat Async::Stop as fatal. The pool type is derived from the class name, and the factory performs the lookup in reverse, trusting the type it receives: the worker only ever passes :thread or :fiber. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
e6ed969 to
2298a9e
Compare
The async dependency and isolation level checks were surfaced through Configuration's validations but implemented in the fiber pool, which also ran them when built. Configuration is where these belong: it validates every supervised path before any process is instantiated, and reports through valid? and check. The pool now just requires the async gem lazily when starting its reactor, keeping setups without fiber workers from ever loading it, and otherwise trusts its caller, like the rest of the internal classes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the ExecutionPools module with a Pool abstract class that the thread and fiber pools inherit from, and that builds the right pool for a worker, mirroring how Supervisor relates to ForkSupervisor and AsyncSupervisor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks so much for this, Rosa! I have merged the PR. I went through the changes carefully and really appreciate the attention to detail here. In particular:
I also traced the lazy-start lifecycle and confirmed that the worker is the sole producer posting into its pool, so the simpler initialization remains appropriate. I ran the focused tests, the full SQLite suite, and RuboCop successfully. Thanks again. This leaves the implementation considerably cleaner and easier to reason about. |
|
Awesome! Huge thanks for reviewing this, @crmne! Really appreciate it. From my side, this should make the main PR ready to be merged now. Do you think you need to test this again, in the same way you tested the original PR? If so, happy to wait for that. |
|
Yes, I think rerunning the benchmark suite is worthwhile. I am away from home and the original runs were done on my desktop, so I cannot produce an exact comparison with those numbers until next week. I can run the complete suite on my laptop in the meantime. Since thread and fiber modes will be benchmarked together on the same machine, the results will still be internally comparable. I just would not compare the absolute numbers directly with the previous desktop results. My preference is to do that now so the merge is not blocked, then repeat it on the desktop next week only if we want an exact comparison with the published results. |
Hey @crmne! While finishing the review of rails#728 (thanks so much for all the work on it and for your patience! 🙏), I ended up with some changes that go beyond style tweaks, so rather than force-pushing them over your branch directly, here's a PR against it so you can see and weigh in on them:
inflightgauge fresh required a mutex + dirty-flag in the worker, an extraUPDATEper claiming poll (up to 10/s per worker at the default polling interval), and had the poller and heartbeat threads racing to update the same row. And once refreshed only on heartbeats, the gauge could be stale by up to a heartbeat interval — better not to show it than to show stale info, especially since the exact count is always available from claimed executions. Workers now registerpool_typeandpool_sizeonce, both constant. With the dirty-tracking gone, the pool callback's only job is waking the worker when capacity frees up, so it gets its oldon_idlename back.initializeagain — nobootoverride or deferred pool options needed — merge defaults unconditionally, and pick the pool type with a single check. Instantiated directly with no pool options, a worker defaults to threads; with both, fibers win (the configuration forbids that combination anyway).threads+fibersrejection, and the async-dependency and isolation-level checks moved from the fiber pool into Configuration itself, where they're surfaced throughvalid?andcheckon every supervised path, before any process is instantiated. Internal classes trust their callers, same as the scheduler does with recurring tasks.SolidQueue::Poolas the pools' base class: both pools shared the capacity ledger (reserve/restore accounting, idle notification, and the reserve-then-schedule shape ofpost), so that now lives in an abstractPoolthat also builds the right pool for a worker viaPool.build, mirroring howSupervisorrelates toForkSupervisorandAsyncSupervisor. Each pool is left with just its scheduling mechanism, and the pool type is derived from the class name. (The oldSolidQueue::Poolback-compat alias is removed early in the branch; the name returns here as the actual abstraction — it was internal API with no remaining references.)on_rejectionbackstop the old pool had. Relatedly, posting now scopes its rescue to the code that runs after capacity is reserved, instead of tracking reservation with a local flag.Each commit is independently green, so the history bisects cleanly. Merging this into your branch will update the main PR. If you'd rather discuss any of these, happy to!
🤖 Generated with Claude Code