CLI: Send site events over socket instead of through events-relay pm2 process#2398
Conversation
…n to CREATED events
|
Status update: we've decided to follow this strategy, but we are currently trying to figure out why the E2E tests fail on macOS (on CI) |
This reverts commit 823492f.
This reverts commit dc4b363.
| } | ||
| childProcess.kill(); | ||
| const pid = childProcess.pid; | ||
| const result = childProcess.kill( 'SIGKILL' ); |
There was a problem hiding this comment.
I was battling failing E2E tests on macOS today and yesterday. The reason they were failing was that the first test suite (e2e/app.test.ts) runs very quickly. So quickly, in fact, that the events subscriber child process only ran for 25ms or so before stopCliEventsSubscriber was called.
The _events command wouldn't even have time to register SIGINT/SIGTERM event listeners in that time, which meant that the process would not be killed. I initially tried to counteract this by hardening the cleanup logic in the runCommand function, not running any async IO before registering the SIGINT/SIGTERM listeners, etc.
Even after I did all that, the tests still failed the same way. This led me to the conclusion that the SIGTERM signal (which is what ChildProcess::kill sends by default on Posix platforms in node) was sent before runCommand was even triggered, while some other async work was happening. It could, for instance, be the translation loading in cli/index.ts, or something that yargs does internally. I guess it could potentially even be an issue with multiple cores executing logic in parallel…
In any case, SIGKILL signals don't allow the child process to perform any cleanup; it's essentially a force kill operation. pm2-axon has logic that gracefully binds to a socket if the process that originally bound to it is no longer running, which is why it's not critical to let the _events child process run the cleanup.
The E2E tests are passing now, and I think this is a pragmatic solution to our problem 👍
📊 Performance Test ResultsComparing dbf3b88 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
|
The performance tests are working, at least 🙂 |
bcotrim
left a comment
There was a problem hiding this comment.
Code LGTM 👍
Didn't find any regressions
|
Thanks, @bcotrim. I just pushed a fix to mute the deprecation warning, too |


Related issues
Proposed Changes
Use the
pm2-axonlibrary to send events over a socket/named pipe instead of using the events-relay child process. The benefit of this approach is that it doesn't break if users runstudio site stop --allfrom their terminal while Studio is also running.Tip
Now that we use a socket, we should start thinking about whether there's a future scenario where Studio can be responsible for binding a server on the socket, and events could be sent directly from CLI processes to Studio.
Testing Instructions
npm startnode dist/cli/main.js site stop --allnode dist/cli/main.js site start --path PATH_TO_SITEwherePATH_TO_SITEis the path to a Studio sitePre-merge Checklist