feat: add exception handler

[heilmela]

@endel any thoughts on this ?

[endel]

Thank you for the PR @heilmela! I like this!

I'll need some time to review this, a few notes for me later:

• The onAuth/onJoin/onLeave methods currently have their own error handling (they don't kill the process), I'm afraid this change may conflict with that, need to add tests for it
• Would be nice to provide which method name the error came from on onUncaughtException, e.g. onUncaughtException(error: Error, methodName: "onCreate" | "onJoin" | ...)
• Would it be possible to catch errors on this.clock's timer events as well?
  • ... and .setSimulationInterval()?
• The user should be able to opt-out of this feature

[heilmela]

Just adding my 2 cents

• we could add the method name or provide specific error types like OnJoinException which just wraps the parent message
  ES2022 added the cause attribute for that so the stack trace is kept e.g.:

// ..catch exception
throw new OnJoinException("Oops", { cause: exception })

User can then basically switch over the exception type when they want to handle each case differently.

• Technically the user is "opted-out" of this feature since the default behaviour is rethrowing the error which would be the same behaviours as before the change. Handling these exceptions is completely optional.

[github-actions]

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

[hunkydoryrepair]

I agree with Endel that the onCreate/onJoin and onAuth exception handling is best the way it is.
We use exceptions heavily in all of these, and it is important to know how they are handled.

Specifically, if we throw during onJoin, the player is never added to the room on onLeave is not called.
But, that's been somewhat broken because onLeave can still get called now before onJoin finishes.

The bigger issue I see is handling exception WITHIN colyseus, not in the user code. This doesn't appear to benefit that at all.

For example, if REDIS calls throw any error when matchmaker uses it, the application will shut down. REDIS likes to throw exceptions. And, perhaps, in some cases a shutdown is the correct thing that needs to happen, but it should happen fully. In the current implementation, it calls onLeave and onDispose for players/rooms, but unfortunately, it is too later to do any asynchronous work, so database saves and anything asynchronous isn't guaranteed to run. If colyseus could handle exceptions BEFORE they become unhandled exceptions, and do a shutdown when necessary, that would be better.

This PR as it is doesn't seem valuable because it isn't catching the errors that the currently the application cannot catch for itself.

[hunkydoryrepair]

One place that needs exception handling is in Room.broadcastPatch

it is currently possible that the application corrupts the Room State. The application should fix that error, but Colyseus can mitigate the damage when this happens by catching errors in broadcastPatch and shutting down the room (corrupted state means it is probably dangerous to continue using it). The alternative is that it becomes an Unhandled Exception and the entire node.js process is shut down.

Similary, where getFullState is called, probably in Room._onMessage where it calls this.sendFullState(client);, it should catch errors and not let the player join.

[hunkydoryrepair]

REDIS calls, by the way, are asynchronous, and generally not awaited in colyseus (would just make things slower), but redis calls should utility .catch() to prevent server shutdown (or initialize a graceful shutdown that is actually graceful, unlike with unhandled exceptions)

[heilmela]

@hunkydoryrepair, it appears there's a misunderstanding. This PR's intention isn't to modify Colyseus's approach to handling exceptions, but rather to enhance exception management in the user space. We've encountered numerous instances where an issue in a single hook caused the entire process, and consequently all rooms, to fail. Although this isn't a fault of Colyseus but ours, improving the API to handle such issues seems beneficial. A significant aspect of Colyseus's value lies in the ease of developing stable products, so I respectfully disagree with the notion that "the user can catch errors himself."

Moreover, I'm puzzled as to why this has evolved into a debate about Colyseus's overall exception handling, particularly regarding Redis.

On a constructive note, your observation about the 'onJoin' is noteworthy. Wouldn't the issue be addressed by implementing the exception proxy post-Colyseus's existing exception handling logic? This way, Colyseus offers a hook-based API for exceptions without disrupting its internal exception handling.