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

Error responsebody truncated when RESPONSE_SET_STATUS_OVER_SEND_ERROR is set #5279

Open
peter-janssen opened this issue Mar 9, 2023 · 3 comments

Comments

@peter-janssen
Copy link

peter-janssen commented Mar 9, 2023

It seems that #5035 introduced some unexpected behaviour. Specifically when the ResponseWriter processes unhandled errors and completes the CompletableFuture<ContainerResponse> responseContext exceptionally. This causes an exception in the writeResponseStatusAndHeaders method of the ResponseWriter when response is processed with the DefaultExceptionMapper.

115    @Override
116    public OutputStream writeResponseStatusAndHeaders(final long contentLength, final ContainerResponse responseContext)
117        throws ContainerException {
118        this.responseContext.complete(responseContext);
119
120        // first set the content length, so that if headers have an explicit value, it takes precedence over this one
121        if (responseContext.hasEntity() && contentLength != -1 && contentLength < Integer.MAX_VALUE) {
122            response.setContentLength((int) contentLength);
123        }
124        // Note that the writing of headers MUST be performed before
125        // the invocation of sendError as on some Servlet implementations
126        // modification of the response headers will have no effect
127        // after the invocation of sendError.
128        final MultivaluedMap<String, String> headers = getResponseContext().getStringHeaders();
...

On line 118 the responseContext is completed again with the provided ContainerResponse. The getResponseContext() method, on line 128, calls get() on the CompletableFuture<ContainerResponse> which results in a ExecutionException which is subsequently wrapped in a ContainerException. This leaves the response in a weird state since the Content-Length header is set on line 122. Ultimately this results in a truncated error message limited to 141 bytes (which is the length of the DefaultExceptionMapper default message: "An exception mapping did not successfully produce and processed a response. Logging the exception propagated to the default exception mapper."). This only happens when RESPONSE_SET_STATUS_OVER_SEND_ERROR is set to true.

I've created a sample project to demonstrate the issue. When using JerseyTest this issue does not occur in the GrizzlyHttpContainer.

P.S. I'd like to challenge the response entity of the DefaultExceptionMapper. The specification only calls for the status code to be set. Shouldn't the response remain empty?

peter-janssen pushed a commit to peter-janssen/reproducers that referenced this issue Mar 9, 2023
@peter-janssen
Copy link
Author

@jansupol Care to take a look?

@peter-janssen
Copy link
Author

I guess this issue could be closed except for the P.S. The specification states the following:

A JAX-RS implementation MUST include a default exception mapping provider that implements ExceptionMapper and which SHOULD set the response status to 500.

Jersey now also sets a response entity (message). Shouldn't this be left empty

@sford
Copy link

sford commented Mar 10, 2024

Jersey now also sets a response entity (message). Shouldn't this be left empty

I created #5558 for a related issue around this. Setting RESPONSE_SET_STATUS_OVER_SEND_ERROR allows post-processing the response in servlet filter because setStatus shouldn't close the response like sendError would. However, the fact that the default exception mapper is adding this message is causing servlet containers to close the response, preventing post-processing of the response even when RESPONSE_SET_STATUS_OVER_SEND_ERROR is set to true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants