Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebSockets Next: investigate error handling #39186

Closed
geoand opened this issue Mar 5, 2024 · 21 comments
Closed

WebSockets Next: investigate error handling #39186

geoand opened this issue Mar 5, 2024 · 21 comments
Assignees
Labels

Comments

@geoand
Copy link
Contributor

geoand commented Mar 5, 2024

Description

We should provide a way for users to handle errors - perhaps in a similar way as JAX-RS / Jakarta REST

Implementation ideas

No response

@cescoffier
Copy link
Member

Here are my thoughts:

I propose the introduction of an @OnError method annotation to enhance the error-handling capabilities of web socket endpoints. This annotation serves as an instruction to the framework, indicating that a specific method should be invoked when certain types of errors occur:

  • Serialization/deserialization errors
  • Connection errors

A web socket endpoint can define multiple @OnError methods, each handling different types of exceptions:

@OnError
void onIOException(IOException e) { ... }

@OnError
void onWhateverException(WhateverException e) { ... }

Each @OnError method corresponds to a specific exception type. However, it's important to note that if two methods handle a class and its parent, the behavior becomes ambiguous.

For comprehensive error handling, @OnError methods accepting Exception or Throwable act as catch-all handlers. They are invoked when no other @OnError method matches the exception type.

@OnError methods are invoked within the WebSocket session scope. This enables them to access the WebSocketConnection and take recovery actions like writing messages. The connection can also be used to close the connection with an error code.

Some errors are terminal, resulting in the termination of the web socket connection. For instance, an IOException during a write operation would lead to such termination.

Furthermore, I propose the ability to define a "global" error handler using a CDI bean that implements WebSocketErrorHandler<? extends Exception>. This global error handler behaves similarly to individual @OnError methods but is invoked when there is no matching handler within the web socket class itself. This approach eliminates the need to replicate error-handling logic across every web socket endpoint, enhancing code maintainability and reducing redundancy.

WDYT?

@cescoffier cescoffier removed their assignment Mar 13, 2024
@geoand
Copy link
Contributor Author

geoand commented Mar 13, 2024

I agree, but with the following caveat:

Furthermore, I propose the ability to define a "global" error handler using a CDI bean that implements WebSocketErrorHandler<? extends Exception>. This global error handler behaves similarly to individual @onerror methods but is invoked when there is no matching handler within the web socket class itself. This approach eliminates the need to replicate error-handling logic across every web socket endpoint, enhancing code maintainability and reducing redundancy.

Why not just allow users to do something like:

public class GlobalErrorHandlers {
   
   @OnError
   public void onWhateverException(WhateverException e)  {

    }
}

as we do in RESTEasy Reactive?

The advantages for something like are:

  • Users don't need to implement another API - they just use the @OnError annotation
  • It allows us to progressively add more features to error handlers - meaning that we in the future we can "inject" more method parameters

@mkouba
Copy link
Contributor

mkouba commented Mar 20, 2024

Each @OnError method corresponds to a specific exception type. However, it's important to note that if two methods handle a class and its parent, the behavior becomes ambiguous.

Hm, does it mean that if an exception thrown is assignable to the method parameter then the callback method should be used? Also what exactly does ambiguous mean? Suppose that we have multiple error handlers that match the thrown exception. Should we use the first one matching and log an error message or?

The connection can also be used to close the connection with an error code.

Currently, we don't support error codes. Should we file a separate issue for this?

The advantages for something like are:

* Users don't need to implement another API - they just use the `@OnError` annotation

* It allows us to progressively add more features to error handlers - meaning that we in the future we can "inject" more method parameters

The only inconsistency here is that other @OnX annotations may only be declared on an endpoint class 🤔. And I think that the same applies to jakarta.websocket.OnError.

@geoand
Copy link
Contributor Author

geoand commented Mar 20, 2024

Hm, does it mean that if an exception thrown is assignable to the method parameter then the callback method should be used?

Yes, that's one of the ways we it in RESTEasy Reactive (the other is to allow the user to specify the type in the annotation itself).

The only inconsistency here is that other @onx annotations may only be declared on an endpoint class

In RESTEasy Reactive they can be global (i.e. for all REST endpoints) or per-class

@mkouba
Copy link
Contributor

mkouba commented Mar 22, 2024

Hm, does it mean that if an exception thrown is assignable to the method parameter then the callback method should be used?

Yes, that's one of the ways we it in RESTEasy Reactive (the other is to allow the user to specify the type in the annotation itself).

Ok, I understand this concept. The question is what to do if multiple error handlers apply to the given type?

Let's take a few steps back and enumerate the cases where a user-defined error handler can be used:

  • callback invocation errors,
  • codecs-related issues, i.e. encoding/decoding problems, no suitable codec found, etc.,
  • error when writing the message obtained from the callback
  • anything else?

Now the other question is: should we create a specific exception for each type of problem or one exception with an error code?

@geoand
Copy link
Contributor Author

geoand commented Mar 22, 2024

Now the other question is: should we create a specific exception for each type of problem or one exception with an error code?

I am in favor of the former because users can then decide which ones to handle and if we make a good class hierachy they could handle them in any granulrity they like.

The question is what to do if multiple error handlers apply to the given type?

JAX-RS has this (fairly) simple rule:

When choosing an exception mapping provider to map an exception,
an implementation MUST use the provider whose generic type is the nearest superclass of the exception

@mkouba
Copy link
Contributor

mkouba commented Mar 22, 2024

JAX-RS has this (fairly) simple rule:

When choosing an exception mapping provider to map an exception,
an implementation MUST use the provider whose generic type is the nearest superclass of the exception

Well, that does not solve multiple providers with the same type? 🤔

@geoand
Copy link
Contributor Author

geoand commented Mar 22, 2024

In that case the priority is taken into account.
And if that does not solve it, there is a final rule that states that user provided code should take precedence over code provided by the framework.
If even that does not solve it, I think we just pick one at random 😃

@mkouba
Copy link
Contributor

mkouba commented Mar 22, 2024

In that case the priority is taken into account. And if that does not solve it, there is a final rule that states that user provided code should take precedence over code provided by the framework. If even that does not solve it, I think we just pick one at random 😃

Hehe, and suddenly a simple rule is not simple anymore :D

@Ladicek
Copy link
Contributor

Ladicek commented Mar 22, 2024

I faced similar problem in SmallRye Fault Tolerance (see here) and went with this simple rule:

The method that declares a most-specific supertype of the actual exception is selected.

Of course, I'm piggybacking on the (relatively complex) selection rules for fallback methods, so I don't have to care about most cases of ambiguity.

If you have per-class and global exception handlers, it seems best to attempt selecting a per-class exception handler first and only if there's none, try to select a global one, using the same rule. Unfortunately, this rule doesn't really resolve all possible cases of ambiguity.

For per-class error handlers, I can see:

  • a class can declare multiple @OnError methods [with different names] that declare the same exception type
  • a class may perhaps even inherit @OnError methods (needs to be defined)

For global error handlers, assuming the same @OnError style of method declarations, except that the methods occur outside of WebSocket endpoint classes, there are:

  • all the ambiguity problems for per-class error handlers
  • multiple classes may declare global error handlers

Also, it seems that certain error handlers may be able to fix the problem and resume normal operation (@OnMessage method throws an exception, which the @OnError may want to convert to an error message and send it back, the WebSocket connection remains usable, this is purely application-level concern), while some would find themselves in a fatal situation (client abruptly disconnects, nothing you can do).

So the error handlers in general need to be able to send replies on the connection. If that was possible using the same approach as @OnMessage methods (that is, returning a value), I guess that would be nice.

It might be useful to distinguish application-level concerns from fatal concerns by different annotations (@OnError vs. @OnFatalError?), but I might be overthinking it here.

@mkouba mkouba self-assigned this Mar 25, 2024
mkouba added a commit to mkouba/quarkus that referenced this issue Mar 26, 2024
@mkouba
Copy link
Contributor

mkouba commented Mar 26, 2024

Here's a draft PR: #39705

The global error handlers are not supported yet but it should not be difficult to add. Done.

For now, we only "catch" encoding/decoding errors and runtime problems (i.e. anything thrown by @OnOpen, @OnXMessage and @OnClose callback invocation).

@cescoffier
Copy link
Member

You should have an exception handler on the WebSocket connection. No?

@cescoffier
Copy link
Member

Yes, there is an exception handler. Be careful as it's called on the event loop, which may not be what you want for the @OnError method.

@mkouba
Copy link
Contributor

mkouba commented Mar 27, 2024

Yes, there is an exception handler. Be careful as it's called on the event loop, which may not be what you want for the @OnError method.

Yes, but for this one I was thinking of another annotation (@OnConnectionError maybe?) because it is a different type of error with different set of fallback operations...

mkouba added a commit to mkouba/quarkus that referenced this issue Mar 27, 2024
- introduce OnError annotation
- add encoding/decoding exceptions
- also support global error handlers
- see quarkusio#39186
mkouba added a commit to mkouba/quarkus that referenced this issue Mar 27, 2024
- introduce OnError annotation
- add encoding/decoding exceptions
- also support global error handlers
- see quarkusio#39186
@mkouba
Copy link
Contributor

mkouba commented Mar 27, 2024

@cescoffier If a callback (e.g. @OnTextMessage) returns Uni should we call the error handler if the returned Uni failed?

mkouba added a commit to mkouba/quarkus that referenced this issue Mar 27, 2024
- introduce OnError annotation
- add encoding/decoding exceptions
- also support global error handlers
- see quarkusio#39186
mkouba added a commit to mkouba/quarkus that referenced this issue Mar 27, 2024
- introduce OnError annotation
- add encoding/decoding exceptions
- also support global error handlers
- see quarkusio#39186
mkouba added a commit to mkouba/quarkus that referenced this issue Mar 27, 2024
- introduce OnError annotation
- add encoding/decoding exceptions
- also support global error handlers
- see quarkusio#39186
@mkouba
Copy link
Contributor

mkouba commented Mar 28, 2024

@cescoffier If a callback (e.g. @OnTextMessage) returns Uni should we call the error handler if the returned Uni failed?

Done #39763

@cescoffier
Copy link
Member

@mkouba sorry for the late reply, but yes, for both uni and multi, if they emit a failure, we should call the error handler.

@mkouba
Copy link
Contributor

mkouba commented Apr 2, 2024

@mkouba sorry for the late reply, but yes, for both uni and multi,

@cescoffier no problem!

if they emit a failure, we should call the error handler.

However, in the docs we currently state "When returning a Multi, Quarkus subscribes to the returned Multi automatically and writes the emitted items until completion, failure, or cancellation. Failure or cancellation terminates the connection.". It does not make sense to call the error handler in this case, or? Or maybe it does not make sense to close the connection unless it's a bi-directional stream?

@mkouba
Copy link
Contributor

mkouba commented Apr 2, 2024

@mkouba sorry for the late reply, but yes, for both uni and multi,

@cescoffier no problem!

if they emit a failure, we should call the error handler.

However, in the docs we currently state "When returning a Multi, Quarkus subscribes to the returned Multi automatically and writes the emitted items until completion, failure, or cancellation. Failure or cancellation terminates the connection.". It does not make sense to call the error handler in this case, or? Or maybe it does not make sense to close the connection unless it's a bi-directional stream?

Hm, maybe it does not make sense to close the connection at all. I mean, even for @OnTextMessage bi-directional stream, there might be @OnOpen or @OnBinaryMessage that could still work properly. So I think it would be reasonable to always call the error handler (which can close the connection if needed).

@cescoffier
Copy link
Member

I agree, it should not close the connection.

gsmet pushed a commit to gsmet/quarkus that referenced this issue Apr 2, 2024
- introduce OnError annotation
- add encoding/decoding exceptions
- also support global error handlers
- see quarkusio#39186

(cherry picked from commit bce9f55)
@mkouba mkouba closed this as completed Apr 19, 2024
ketola pushed a commit to ketola/quarkus that referenced this issue May 1, 2024
- introduce OnError annotation
- add encoding/decoding exceptions
- also support global error handlers
- see quarkusio#39186
franz1981 pushed a commit to franz1981/quarkus that referenced this issue May 20, 2024
- introduce OnError annotation
- add encoding/decoding exceptions
- also support global error handlers
- see quarkusio#39186

(cherry picked from commit bce9f55)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants