Timeout site stop --all after 6s when quitting#2475
Conversation
📊 Performance Test ResultsComparing bbdb78f vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
epeicher
left a comment
There was a problem hiding this comment.
Thanks @fredrikekelund! I have tested it, and I can see that the application exits cleanly after different amounts of time. I left a minor comment, but this can progress anyway. LGTM!
| if ( shouldStopSitesOnQuit ) { | ||
| event.preventDefault(); | ||
| stopAllServers( true ) | ||
| stopAllServers( true, 6_000 ) |
There was a problem hiding this comment.
Nit: I think adding a constant with a meaningful name would help to identify what this number is
There was a problem hiding this comment.
I think the function argument name already provides similar clarity. Extracting the value into a constant would also introduce a jump for the reader between where the value is defined and this, the only place where it's used. So, in this particular case, I'll go ahead and land it as is 👍

Related issues
Proposed Changes
When Studio quits, the
stopAllServersfunction runs thesite stop --allCLI command to stop running sites. This function resolves if the command fails or if the Node.jsChildProcessinstance emits anerrorevent.Unfortunately, when testing the Studio 1.7.0 Intel build on my Apple Silicon laptop (using Rosetta emulation) none of these things appear to happen. I don't yet know what happens for regular macOS Intel users, but there are some indications that
stopAllServersdoes resolve. In any case, it's likely not working for me because the bundled node runtime initializes but can't proceed, getting stuck in an intermittent state.To mitigate against similar issues in the future, this PR adds an optional timeout to the
stopAllServersfunction that kills the child process and resolves the promise.Testing Instructions
npm startPre-merge Checklist