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

CompletionStage behaviour in case of WebApplicationException #4463

Open
daijithegeek opened this issue May 12, 2020 · 8 comments
Open

CompletionStage behaviour in case of WebApplicationException #4463

daijithegeek opened this issue May 12, 2020 · 8 comments
Labels
enhancement New feature or request notabug not a bug, will not be fixed
Milestone

Comments

@daijithegeek
Copy link
Contributor

daijithegeek commented May 12, 2020

Hello,

I'm faced with a case where I'm not sure what the behavior should be.
When throwing a WebApplicationException, jersey automatically translates the exception in its corresponding response.

While using CompletionStage, I would expect the behavior to be similar. In practice, it translates to an internal server error. The JAX-RS specification is not very clear on that particular point

Consider the following case :

@GET
@Path("failAsync/later")
public CompletionStage<String> failAsyncLater() {
  CompletableFuture<String> fail = new CompletableFuture<>();
  failLater(fail, new IllegalStateException("Uh-oh"));

  // We re-throw a WebApplicationException .exceptionally.
  // This has the side effect of wrapping the WebApplicationException inside a CompletionException
  return fail.exceptionally(ex -> {
    throw new WebApplicationException("OOPS", Response.Status.NOT_ACCEPTABLE.getStatusCode());
  });
}

private void failLater(CompletableFuture<?> cf, Throwable t) {
  new Thread(() -> {
    try {
      Thread.sleep(500);
      cf.completeExceptionally(t);
    }
    catch (InterruptedException ex) {
      // nothing to do, let the thread die.
    }
  ).start();
}

I would actually expect to get an HTTP 406, instead of a 500.

Can you clarify the current behavior and the way this kind of error is supposed to be handled ?

Thanks a lot!

@jansupol jansupol added this to the 2.32 milestone May 13, 2020
@jansupol
Copy link
Contributor

What is happening is

public CompletionStage<String> failAsyncLater() {
  throw new CompletionException(new WebApplicationException("OOPS", Response.Status.NOT_ACCEPTABLE.getStatusCode());

Now, it is possible to map the nested exception in Jersey, however, there may be a use case where the user would not want to decapsulate the nested exception. In that case, Jersey would not be able to help.

In your case, there are two possible solutions.

  1. Do:
public CompletionStage<String> failAsyncLater() {
  CompletableFuture<String> fail = new CompletableFuture<>();
  failLater(fail, new IllegalStateException("Uh-oh"));
  return fail;
}

private void failLater(CompletableFuture<?> cf, Throwable t) {
  new Thread(() -> {
    try {
      Thread.sleep(500);
      cf.completeExceptionally(new WebApplicationException("OOPS", Response.Status.NOT_ACCEPTABLE.getStatusCode());
    }
    catch (InterruptedException ex) {
      // nothing to do, let the thread die.
    }
  ).start();
}
  1. Create a user-defined ExceptionMapper for the CompletionException.

@jansupol jansupol added the notabug not a bug, will not be fixed label Jun 29, 2020
@jansupol
Copy link
Contributor

  1. The easiest at the moment seems
        public CompletionStage<Response> failAsyncLater() {
            CompletableFuture<Response> fail = new CompletableFuture<>();
            failLater(fail, new IllegalStateException("Uh-oh"));
            return fail.exceptionally(ex -> {
                return Response.status(406).build();
            });
        }

@jansupol
Copy link
Contributor

The combination of CompletionException(WebApplicationException) does not look like one user usually wants to map on his own, maybe for this case would make sense the CompletionException to be unwrapped, though.

@dansiviter
Copy link

Can #3978 also be progressed as they are different sides of the same async coin?

@daijithegeek
Copy link
Contributor Author

The combination of CompletionException(WebApplicationException) does not look like one user usually wants to map on his own, maybe for this case would make sense the CompletionException to be unwrapped, though.

I don't know either. On my side I asked the question, because I was surprised and I would have expected the behavior to be coherent with the sync exception handling (unwrap and convert to response)
Thanks for the two handling examples, we use an ExceptionMapper for now.

@twbecker
Copy link

I just hit this as well and was quite surprised. Given the nature of CompletionException, I find it hard to think of a scenario where you wouldn't want to unwrap it. The spec seems strangely mum on what exactly the behavior should be here, but FWIW RESTeasy is documented to behave as if the resource method threw the underlying exception directly.

@jansupol
Copy link
Contributor

@twbecker Can you be more specific about your issue? What Jersey version do you use? What is the exception you hit? Can you share a stacktrace?

@twbecker
Copy link

twbecker commented May 1, 2024

@jansupol Using Jersey 3.0.12. I didn't hit an exception in Jersey, I'm talking about how exceptions are handled by Jersey. Basically, I'd expect the following resource methods to return identical responses, but they don't:

@Path("/")
public class Resource {

    @GET
    @Path("/sync")
    public Response synchronous() {
        throw new ServiceUnavailableException();
    }

    @GET
    @Path("/async")
    public CompletionStage<Response> asynchronous() {
        return CompletableFuture.supplyAsync(() -> {
            throw new ServiceUnavailableException();
        });
    }
}

Specifically, the exception from the async method does not go through the ExceptionMapper machinery as expected because it winds up getting wrapped by a CompletionException. From an application developer's perspective that is odd because the exception being handled is not the one that was thrown by the application.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request notabug not a bug, will not be fixed
Projects
None yet
Development

No branches or pull requests

4 participants