Refactor: extract worker process into separate class [changelog skip] #2374
Conversation
|
Thank you for your work on this.
Nice way of saying... Having recently spent a few weeks away from thinking about anything Puma, I would agree. Any thoughts on removing the current Being unable to run fork locally (hope to fix that soon), I can't really test this, but CI is passing. 👍 Lastly, the gem version/update issue would be helped by this. If |
This class represents a worker from the perspective of the puma master process. It provides methods for controlling the process, but doesn't contain the logic actually executed by the worker. In preparation for creating a new class that encapsulates the worker logic, we're renaming this one to WorkerHandle.
Before, all functionality of the worker processes was defined in the Cluster class. In preparation for making it possible to start worker processes outside of the context of a Cluster instance, we move the worker functionality into a new class. This has the additional benefit of delineating exactly the dependencies of the worker processes, namely the Launcher, options hash, and the pipes used for inter-process communication.
cjlarose
force-pushed
the
extract-worker-class
branch
from
8009aa1 to
bef62c5
Compare
I agree that |
|
I was mostly concerned with the namespace issue, and the embedded class in Cluster. As to JFYI, there are intermittent failures with non MRI jobs in CI, especially on macOS. AFK. Thanks, Greg |
|
I'm a complete outsider to the Puma codebase, but just tried to understand the naming difference you discussed... So if I understand this correctly the What about renaming apologies if this doesn't make any sense and I'm butting in to a discussion that's just way over my head :) I know naming is hard, but I hope that my outsider's perspective can be helpful to see things with a fresh outlook. |
|
I'm kind of shocked that this clean of an extract was possible. I'm not terribly bothered about the current naming. Need to run through it in more detail but in concept I'm 100% behind this. |
| fork_worker = @options[:fork_worker] && index == 0 | ||
|
|
||
| @workers = [] | ||
| if !@options[:fork_worker] || fork_worker |
There was a problem hiding this comment.
I never realized this conditional was always true 😞
| ensure | ||
| @worker_write << "t#{Process.pid}\n" rescue nil | ||
| @worker_write.close | ||
| new_worker = Worker.new index: index, |
There was a problem hiding this comment.
Pretty delicious interface here 👍
4 participants
Description
My original motivation for extracting code related to the worker processes is the change described in #1875 (comment). I want to be able to create the first worker process (assuming
fork_workeris on) by firstforking from the master, then immediatelyexecing into a new process definition with a separate memory space.My proof-of-concept in that comment technically works but it's messy because the logic used by the worker process is woven in together with the logic used by the puma master process in
Puma::Cluster. Worker processes implicitly rely on several instance variables used inPuma::Cluster, so for the new process to spin up a new worker, it needs to first spin up a dummy instance ofPuma::Cluster.To make it a little bit cleaner, I pulled out the worker logic into a new
Puma::Cluster::Workerclass. The initializer lays out everything that's necessary to construct a new worker process. My hope is that it's clear that this refactor is worth having regardless of the original motivation for it because it makes dependencies of the worker process clear and lowers the cognitive overhead of having worker logic and cluster logic in the same class definition.Your checklist for this pull request
[changelog skip]or[ci skip]to the pull request title.[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.