Is it a good idea to kill the process on errors ?

[heilmela]

Context

Now and then im dealing with bugs in room hooks which throw uncaught errors. Developers in our team sometimes introduces rather harmless bugs which in the worst case scenario introduce inconsistent state.

Current Behaviour

Looking at the following snippet in the Matchmaker, colyseus kill the process when such errors occur

colyseus/packages/core/src/utils/Utils.ts

Line 27 in 27ad5c6

process.on('uncaughtException', (err) => {

It seems to me that its rather brutal to kill all rooms by ending the process when only a single room might have produced this uncaught error - the nature of developing complex room logic and state updates its absolute unrealistic that none of this stuff ever causes some uncaught exceptions. Given the case that a single colyseus process handles several hundred small rooms this would mean every of this rooms gets killed because of a single problematic room -- Why is that even necessary ?

It degrades the production and dev experience of colyseus imensly since not only we have to implement a lot of logic to prevent errors but at the end production we sometimes have phases until a hotfix where colyseus constantly crashes because of some very lightweight bugs.

Im aware that generally speaking node once considered removing this handler completely since its rather crude to prevent the process from exiting here, but in terms of the long running colyseus process where multiple stateful rooms are hosted by a single process i cant get over the fact that a single malicious room can end other rooms with this.

[endel]

Hi @heilmela, thank you for your feedback. This could be a nice addition, if it's possible to implement this without an extra overhead for users that don't need it it would be awesome.

Just FYI this default behavior for graceful shutdown can be disabled by providing the gracefullyShutdown: false option. By default, though, Node.js will print the stack trace and terminate the process.

[heilmela]

Thanks for the reply, im gonna checkout if disabling gracefulShutdown is an option. Regarding the rest of the discussion, im not sure if its a good idea to prevent the process from shutting down anyway when an uncaughtException is thrown. The nature of having a single process managing multiple rooms implies this kind of coupling. I guess that would be one reason to spawn processes or use threads so that there is some kind of isolation and memory safeguards sucht that rooms can not kill each other that easily.

[heilmela]

@endel would you be open to accept a PR which simply adds a onError hook, such that other lifecycle hooks like onCreate etc. do not throw but let the onError hook decide what to do ?
This would at least make error handling for hook logic not propagate into the node process handler such that hook errors can be handled separatly from uncaught exceptions in general. The default behaviour of this onError would be a simple rethrow so the change is fully backwards compatible

[endel]

HI @heilmela, it could be a nice addition!

I'm curious to see how this would impact the performance before accepting it - perhaps we could use onUncaughtException() to be the equivalent of uncaughtException event? Cheers

[heilmela]

@endel its a bit hard to contribute because the project throws a couple of errors when i pull main and try to run tests.
Part of the problem seems to be that ts-node does not take the monorepo symlinked deps (and for some reason c8 does not find a dep semver)
Could you add engines on the package.json and is there a reason your not pushing a lock file (it looks like your using yarn but i couldnt make it work with any package manager i tried).