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

AsyncContext completion attempt after AsyncListener.onError is called #5675 #5773

Draft
wants to merge 1 commit into
base: 2.x
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,14 @@ public void run() {
}
}

static boolean isIOException(Throwable t) {
boolean ioException = false;
while (t != null && !(ioException = t instanceof IOException)) {
t = t.getCause();
}
return ioException;
}

/**
* Get the Jersey server runtime background scheduler.
*
Expand Down Expand Up @@ -669,11 +677,18 @@ public OutputStream getOutputStream(final int contentLength) throws IOException

} catch (final Throwable ex) {
if (response.isCommitted()) {
Copy link
Member

@joakime joakime Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if (response.isCommitted()) { check is actually kinda racy.
The response could be failed in a different thread (eg: from an HTTP/2 goaway), causing the response to be flagged as committed as part of its stream close handling.
This could be false when you check, and then a microsecond later its true.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if there is IOException it does not really matter if the response was sent or not, because we cannot respond. Probably we need to skip the finally block in any case for this type of exception.

/**
* We're done with processing here. There's nothing we can do about the exception so
* let's just log it.
*/
LOGGER.log(Level.SEVERE, LocalizationMessages.ERROR_WRITING_RESPONSE_ENTITY(), ex);
if (isIOException(ex)) {
skipFinally = true;
LOGGER.log(Level.WARNING, "Response was sent, but there is IO issue", ex);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was an issue after the response was committed.
You don't know if the response was sent or not. (it could be sitting in a buffer somewhere locally, not even sent yet)
You just know you cannot change the response anymore.

// Connection is broken. Re-throw to the web container to remove the connection.
throw new MappableException("Response was sent, but there is IO issue", ex);
} else {
/**
* We're done with processing here. There's nothing we can do about the exception so
* let's just log it.
*/
LOGGER.log(Level.SEVERE, LocalizationMessages.ERROR_WRITING_RESPONSE_ENTITY(), ex);
}
} else {
skipFinally = true;
if (ex instanceof RuntimeException) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* Copyright (c) 2024 Oracle and/or its affiliates. All rights reserved.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0, which is available at
* http://www.eclipse.org/legal/epl-2.0.
*
* This Source Code may also be made available under the following Secondary
* Licenses when the conditions for such availability set forth in the
* Eclipse Public License v. 2.0 are satisfied: GNU General Public License,
* version 2 with the GNU Classpath Exception, which is available at
* https://www.gnu.org/software/classpath/license.html.
*
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
*/

package org.glassfish.jersey.server;

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.io.IOException;
import java.io.UncheckedIOException;
import java.net.SocketException;

import org.junit.jupiter.api.Test;

public class ServerRuntimeTest {

@Test
public void ioExceptionInRoot() {
assertTrue(ServerRuntime.isIOException(new IOException()));
assertTrue(ServerRuntime.isIOException(new IOException(new RuntimeException())));
}

@Test
public void ioExceptionInCause() {
assertTrue(ServerRuntime.isIOException(new RuntimeException(new IOException())));
assertTrue(ServerRuntime.isIOException(new RuntimeException(new RuntimeException(new IOException()))));
}

@Test
public void unckeckedIOExceptionInCause() {
assertTrue(ServerRuntime.isIOException(new UncheckedIOException(new SocketException())));
assertTrue(ServerRuntime.isIOException(new RuntimeException(new UncheckedIOException(new SocketException()))));
}

@Test
public void noIOException() {
assertFalse(ServerRuntime.isIOException(new RuntimeException()));
assertFalse(ServerRuntime.isIOException(new RuntimeException(new RuntimeException())));
}

@Test
public void nullException() {
assertFalse(ServerRuntime.isIOException(null));
}

@Test
public void nullCause() {
assertFalse(ServerRuntime.isIOException(new RuntimeException((Throwable) null)));
}
}