Fix: Prevent server process from hanging during stop operation - #1067
Conversation
fredrikekelund
left a comment
fredrikekelund
left a comment
There was a problem hiding this comment.
Does what it says on the tin 👍
I'll create a separate issue for investigating why the Remote Data Blocks plugin throws an error like this, but this is still a great improvement.
| if ( process && process.pid ) { | ||
| require( 'process' ).kill( process.pid, 'SIGKILL' ); | ||
| } else { | ||
| if ( ! isResolved ) { | ||
| isResolved = true; | ||
| resolve(); | ||
| } | ||
| } |
There was a problem hiding this comment.
| if ( process && process.pid ) { | |
| require( 'process' ).kill( process.pid, 'SIGKILL' ); | |
| } else { | |
| if ( ! isResolved ) { | |
| isResolved = true; | |
| resolve(); | |
| } | |
| } | |
| if ( process.pid ) { | |
| kill( process.pid, 'SIGKILL' ); | |
| } else { | |
| isResolved = true; | |
| resolve(); | |
| } |
Nit, but we already know that process is defined at this point, and we also know that isResolved is false because this callback returns early if that's the case.
I also don't see a good reason to use the require pattern here – we might as well do import { kill } from 'process';
| } | ||
| resolve(); | ||
| } ); | ||
| process.kill(); |
There was a problem hiding this comment.
process.kil() returns a boolean per the Electron docs. Is that a useful signal for us maybe..?
There was a problem hiding this comment.
That's a nice catch!
I've made some adjustments to use the return value from process.kill() instead of the TIMEOUT.
Could you take another look, please? 🙏
fredrikekelund
left a comment
fredrikekelund
left a comment
There was a problem hiding this comment.
Nice 👍 New solution also works as expected.
| } ); | ||
| process.kill(); | ||
| // process.kill() returns false if we're not able to kill the process. | ||
| if ( ! process.kill() && ! isResolved ) { |
There was a problem hiding this comment.
| if ( ! process.kill() && ! isResolved ) { | |
| if ( ! process.kill() ) { |
I can't really imagine a scenario in which isResolved would be true at this point 🤔
There was a problem hiding this comment.
I guess I was being extra careful with that condition, but it doesn't make sense anymore with the new approach.
I have removed isResolved check and works as expected.
Thanks for the feedback!
| reject( new Error( `Site server process exited with code ${ code } upon stopping` ) ); | ||
| return; | ||
| if ( ! isResolved ) { | ||
| isResolved = true; |
There was a problem hiding this comment.
| isResolved = true; |
Nit, but given that we're only listening to the exit event once, do we really need to modify isResolved here?
* prevent the server from hanging on stop * adjust forcekill process * adjust forcekill process * remove isResolved check
3 participants
Related issues
Proposed Changes
Testing Instructions
Follow the steps mentioned in the issue
More details on reproducing the issue (thanks @ppolo99): p1741865890679379/1741864717.468099-slack-C04GESRBWKW
Pre-merge Checklist