Don't share server between worker 0 and descendants on refork#3602
Don't share server between worker 0 and descendants on refork#3602
Conversation
f68e078 to
ec3242b
Compare
b7fc17f to
92816b5
Compare
6854dcf to
0438488
Compare
| pipes[:wakeup] = @wakeup | ||
| end | ||
|
|
||
| server = start_server if preload? |
There was a problem hiding this comment.
Now that we can pass the preloaded app into the new worker, we can consistently start the server there instead.
5439297 to
461fd56
Compare
| attr_reader :index, :master | ||
|
|
||
| def initialize(index:, master:, launcher:, pipes:, server: nil) | ||
| def initialize(index:, master:, launcher:, pipes:, app: nil) |
There was a problem hiding this comment.
There are two options here:
- Continue to accept
serverand simply ensure that@appis initialized with@server.appif so. Then, callingstart_serverin#spawn_workerwill use the preloaded worker 0 app to start a new server which we can give to the new worker. This would then mirror the currentCluster#spawn_workerimplementation. - Let the worker consistently be responsible for starting its own server, and instead accept the preloaded
appwhere needed, which is what we really intend to preload. This makes the whole inheritance chain much easier to grok imo, and introduces a clear separation of responsibilities.
37dee58 to
7982fe9
Compare
| def test_application_is_loaded_exactly_once_if_using_fork_worker | ||
| cli_server "-w #{workers} --fork-worker test/rackup/write_to_stdout_on_boot.ru" | ||
|
|
||
| get_worker_pids | ||
| loading_app_count = @server_log.scan("Loading app").length | ||
| assert_equal 1, loading_app_count # loaded in worker 0 | ||
|
|
||
| @server_log.clear | ||
| Process.kill :SIGURG, @pid | ||
|
|
||
| get_worker_pids 1, workers - 1 | ||
| loading_app_count = @server_log.scan("Loading app").length | ||
| assert_equal 0, loading_app_count | ||
| end | ||
|
|
||
| def test_application_is_loaded_exactly_once_if_using_preload_app_with_fork_worker | ||
| cli_server "-w #{workers} --preload --fork-worker test/rackup/write_to_stdout_on_boot.ru" | ||
|
|
||
| get_worker_pids | ||
| loading_app_count = @server_log.scan("Loading app").length | ||
| assert_equal 1, loading_app_count # loaded in master | ||
|
|
||
| @server_log.clear | ||
| Process.kill :SIGURG, @pid | ||
|
|
||
| get_worker_pids 1, workers - 1 | ||
| loading_app_count = @server_log.scan("Loading app").length | ||
| assert_equal 0, loading_app_count | ||
| end |
There was a problem hiding this comment.
These pass on master as well, I added it to ensure I don't regress on this behaviour. I've also done some puts debugging to verify this manually.
|
@nateberkopec Can this be considered for v7? |
|
Is this an alternative to #3643? |
|
Or is it an alternative to #3629? |
Neither actually, this is purely to address #3599. mold_worker should also benefit from this, but I'm going to only focus my testing on fork_worker which is the topic of the issue. I'll redo the load testing comparison in my last comment on the issue now that @MSP-Greg's long-tail fix and my other fix to address the issue are on master, to confirm that this is still needed (I suspect it is). Edit: This is still needed: #3599 (comment) |
78c893c to
4e66b6c
Compare
|
Merging because CI is green (except for the Bundler v4 failures on main) and I've left more than enough evidence in the form of benchmarks in #3599 that this results in a significant performance improvement to fork_worker. |
Description
Closes #3599
Context:
ClusterandWorkersubclassRunnerRunnerhereRunner#start_serverIn ordinary cluster mode, when forking workers from the master process:
Worker#runso that the app is loaded in each worker runnerWhen
fork_workeris enabled, only worker 0 follows the above. Then, when forking workers from worker 0:fork_worker. I've attempted to explain in a comment why I opted for the implementation in this PR.Your checklist for this pull request
[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.