Using worker_threads that keep running indefinitely fails assertion on SIGTERM #23315
Comments
KishanBagaria
changed the title
|
@KishanBagaria There is some existing incompatibility with |
|
I would but I need to use native modules. I also have to move my node-mac-contacts code to a worker thread from the renderer process since it's compute heavy :) |
|
@KishanBagaria I believe I'm having the same issue: #23366 |
|
Is there a way I can silence this by monkey patching or something? |
|
@KishanBagaria we are working around it by monkey-patching You will have to audit all usages of Initially, we were calling Still, this is a bad workaround due to the reasons mentioned above. I hope someone from the Electron team can take an in-depth look at this issue. |
|
I'm this boat as well - while trying to refactor some blocking native logic out of the main thread for performance reasons, I'm now encountering an app crash every time I exit the app because I followed the advice in https://www.electronjs.org/docs/tutorial/performance and used Node.js workers. From where I'm sitting, it looks to me like there is no way to have multithreaded native logic without encountering this error. Is that accurate? |
|
No way around, my code is waiting enough of time to prevent race condition: async function stopWorker(id) {
const worker = wikiWorkers[id];
if (!worker) {
logger.warning(
`No worker for ${id}. No running worker, means maybe worker failed to start`
);
return Promise.resolve();
}
return (
new Promise(resolve => {
worker.postMessage({ type: 'command', message: 'exit' });
worker.once('exit', resolve);
worker.once('error', resolve);
})
.timeout(100)
.catch(error => {
logger.info(`Wiki-worker have error ${error} when try to stop`); // this never happens
return worker.terminate();
})
.then(() => {
delete wikiWorkers[id]; // this is useless, but I tried
logger.info(`Wiki-worker for ${id} stopped`);
})
);
}
const tasks = [];
for (const id of Object.keys(workers)) {
tasks.push(stopWiki(id));
}
await Promise.all(tasks);
await Promise.delay(5000);And in worker if (!isMainThread) {
startNodeJSWiki();
parentPort.once('message', async message => {
if (typeof message === 'object' && message.type === 'command' && message.message === 'exit') {
process.exit(0);
}
});
}and in electron app.on('before-quit', async event => {
logger.info('Quitting worker threads and watcher.');
await Promise.all([stopAllWiki(), stopWatchAllWiki()]);
logger.info('Worker threads and watchers all terminated.');
logger.info('Quitting I18N server.');
clearMainBindings(ipcMain);
logger.info('Quitted I18N server.');
// https://github.com/atom/electron/issues/444#issuecomment-76492576
// if (process.platform === 'darwin') {
// const win = mainWindow.get();
// if (win) {
// logger.info('App force quit on MacOS');
// win.forceClose = true;
// }
// }
app.exit(0);
});
app.on('quit', async () => {
logger.info('App quit');
});All of this are not working. Always |
|
I tried @flotwig 's solution, seems it is not working. |
|
This issue seems like it's present in Node.js v12 but not v14 - i can't repro it after #25249 but can in versions prior |
|
@codebytere that is in electron v11 ? I will try that later |
|
Tried with Electron 11.0.0-beta13. It crashes even when all threads terminate prior to calling app.quit(), like described in #26103 it's enough to have had a thread running at any point to make Electron crash on exit. |
|
I'm fairly certain it's this commit: nodejs/node@b6738bc that would need to be backported to |
|
I can confirm this is fixed in v12 nightly, but v12 drop support for non-NAPI native addons, so I have to disable asar to use those native packages. |
|
Would be great to have this backported! 🎉 For us, Sentry is getting a lot of bad crash dumps because of this. |
|
Having the same issue. Blocked from everywhere. Can't use web workers because of the need to use native modules and it can't seem to load any kind of external module. Can't use worker threads because of this issue. So frustrated not being able to move forward. |
|
A different stack trace for the same issue with a testing build of Electron: |
|
@indutny-signal I think upgrade to v12 beta solves this. |
|
@linonetwo Why beta? The stable version was released a long time ago. And yeah it's fixed because of Node ABI v83. |
|
It still isn't fixed in the latest v11 release which was published a week ago. |
|
@indutny-signal You're never gonna get a fix on v11 because it's using older Node ABI (v72). That will never change. (As far as I know according to the information) |
|
@m4heshd of course it can be fixed. Just need to fix node v12 first 😂 (See: nodejs/node#38010) |
|
@indutny big props. Now I'm glad I added that last part on my comment 😂. Wasted countless amounts of hours because of this. Hope It'll get merged soon. But still, not sure about the Electron side of it. Unless you integrate that too. |
|
@m4heshd I don't anticipate any problems with the electron release, since it'd be just a version bump for node that they use. That being said only time will show if I was right! |
|
@indutny Agreed. Well, hoping for the best. 🤞🏼 (Still wondering why devs on Electron guaranteed that It's unfixable though 🥴) |
The Environment is created after the Isolate and its platform data, so it also needs to be torn down before it. This fixes the crash mentioned below. (Thanks to Fedor Indutny for debugging this!) Fixes: electron#23315
|
@m4heshd does this release fix the problem for you? |
Well you've apparently done the impossible. 😁 But I migrated all of my projects to v12 as soon as I discovered this issue. Still, thank you because It's gonna benefit a lot of devs. slowclaps 👏🏽 |
|
Sounds like this is fixed, so I'll close the issue. Let me know if I'm mistaken and I can reopen. |
Issue Details
Actual Behavior
An electron app that uses a worker thread that runs indefinitely in the main process (because of a server listening on a port etc.) throws the following error on exit:
Expected Behavior
It shouldn't throw the error.
To Reproduce
Couldn't repro this in Electron Fiddle but the following should work:
worker-exit-issue.jsRun
electron worker-exit-issue.jsQuit the Electron app that runs from Dock or equivalent. (CTRL+C/SIGINT will not throw the error)
You'll see the above error printed in the console:
This doesn't happen if I run
node worker-exit-issue.jsThe text was updated successfully, but these errors were encountered: