Skip to content

8301255: Http2Connection may send too many GOAWAY frames #3686

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

8301255: Http2Connection may send too many GOAWAY frames #3686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/java.net.http/share/classes/jdk/internal/net/http/Http2ClientImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,10 +213,10 @@ void deleteConnection(Http2Connection c) {

private EOFException STOPPED;
void stop() {
synchronized (this) {stopping = true;}
if (debug.on()) debug.log("stopping");
STOPPED = new EOFException("HTTP/2 client stopped");
STOPPED.setStackTrace(new StackTraceElement[0]);
synchronized (this) {stopping = true;}
do {
connections.values().forEach(this::close);
} while (!connections.isEmpty());
Expand Down
128 changes: 97 additions & 31 deletions src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
import java.io.EOFException;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.VarHandle;
import java.net.InetSocketAddress;
import java.net.ProtocolException;
import java.net.URI;
Expand Down Expand Up @@ -287,7 +289,10 @@ public void onMaxHeaderListSizeReached(long size, int maxHeaderListSize) throws
}
}

volatile boolean closed;
private static final int HALF_CLOSED_LOCAL = 1;
private static final int HALF_CLOSED_REMOTE = 2;
private static final int SHUTDOWN_REQUESTED = 4;
volatile int closedState;

//-------------------------------------
final HttpConnection connection;
Expand Down Expand Up @@ -703,13 +708,15 @@ final int maxConcurrentServerInitiatedStreams() {
}

void close() {
Log.logTrace("Closing HTTP/2 connection: to {0}", connection.address());
if (connection.channel().isOpen()) {
GoAwayFrame f = new GoAwayFrame(0,
ErrorFrame.NO_ERROR,
"Requested by user".getBytes(UTF_8));
// TODO: set last stream. For now zero ok.
sendFrame(f);
if (markHalfClosedLocal()) {
if (connection.channel().isOpen()) {
Log.logTrace("Closing HTTP/2 connection: to {0}", connection.address());
GoAwayFrame f = new GoAwayFrame(0,
ErrorFrame.NO_ERROR,
"Requested by user".getBytes(UTF_8));
// TODO: set last stream. For now zero ok.
sendFrame(f);
}
}
}

Expand Down Expand Up @@ -771,18 +778,17 @@ Throwable getRecordedCause() {
}

void shutdown(Throwable t) {
if (debug.on()) debug.log(() -> "Shutting down h2c (closed="+closed+"): " + t);
if (closed == true) return;
synchronized (this) {
if (closed == true) return;
closed = true;
}
int state = closedState;
if (debug.on()) debug.log(() -> "Shutting down h2c (state="+describeClosedState(state)+"): " + t);
if (!markShutdownRequested()) return;
cause.compareAndSet(null, t);
if (Log.errors()) {
if (!(t instanceof EOFException) || isActive()) {
if (t!= null && (!(t instanceof EOFException) || isActive())) {
Log.logError(t);
} else if (t != null) {
Log.logError("Shutting down connection: {0}", t.getMessage());
} else {
Log.logError("Shutting down connection");
}
}
client2.deleteConnection(this);
Expand Down Expand Up @@ -966,7 +972,7 @@ private String checkMaxOrphanedHeadersExceeded(HeaderFrame hf) {
}

final void dropDataFrame(DataFrame df) {
if (closed) return;
if (isMarked(closedState, SHUTDOWN_REQUESTED)) return;
if (debug.on()) {
debug.log("Dropping data frame for stream %d (%d payload bytes)",
df.streamid(), df.payloadLength());
Expand All @@ -976,7 +982,7 @@ final void dropDataFrame(DataFrame df) {

final void ensureWindowUpdated(DataFrame df) {
try {
if (closed) return;
if (isMarked(closedState, SHUTDOWN_REQUESTED)) return;
int length = df.payloadLength();
if (length > 0) {
windowUpdater.update(length);
Expand Down Expand Up @@ -1076,7 +1082,8 @@ private void handleConnectionFrame(Http2Frame frame)
}

boolean isOpen() {
return !closed && connection.channel().isOpen();
return !isMarked(closedState, SHUTDOWN_REQUESTED)
&& connection.channel().isOpen();
}

void resetStream(int streamid, int code) {
Expand Down Expand Up @@ -1189,11 +1196,13 @@ private void protocolError(int errorCode, String msg)
String protocolError = "protocol error" + (msg == null?"":(": " + msg));
ProtocolException protocolException =
new ProtocolException(protocolError);
framesDecoder.close(protocolError);
subscriber.stop(protocolException);
if (debug.on()) debug.log("Sending GOAWAY due to " + protocolException);
GoAwayFrame frame = new GoAwayFrame(0, errorCode);
sendFrame(frame);
if (markHalfClosedLocal()) {
framesDecoder.close(protocolError);
subscriber.stop(protocolException);
if (debug.on()) debug.log("Sending GOAWAY due to " + protocolException);
GoAwayFrame frame = new GoAwayFrame(0, errorCode);
sendFrame(frame);
}
shutdown(protocolException);
}

Expand Down Expand Up @@ -1226,9 +1235,11 @@ private void handlePing(PingFrame frame)
private void handleGoAway(GoAwayFrame frame)
throws IOException
{
shutdown(new IOException(
connection.channel().getLocalAddress()
+": GOAWAY received"));
if (markHalfClosedLRemote()) {
shutdown(new IOException(
connection.channel().getLocalAddress()
+ ": GOAWAY received"));
}
}

/**
Expand Down Expand Up @@ -1342,7 +1353,7 @@ <T> void putStream(Stream<T> stream, int streamid) {
// to prevent the SelectorManager thread from exiting until
// the stream is closed.
synchronized (this) {
if (!closed) {
if (!isMarked(closedState, SHUTDOWN_REQUESTED)) {
if (debug.on()) {
debug.log("Opened stream %d", streamid);
}
Expand All @@ -1353,7 +1364,6 @@ <T> void putStream(Stream<T> stream, int streamid) {
}
if (debug.on()) debug.log("connection closed: closing stream %d", stream);
stream.cancel();

}

/**
Expand Down Expand Up @@ -1491,7 +1501,7 @@ void sendFrame(Http2Frame frame) {
}
publisher.signalEnqueued();
} catch (IOException e) {
if (!closed) {
if (!isMarked(closedState, SHUTDOWN_REQUESTED)) {
Log.logError(e);
shutdown(e);
}
Expand All @@ -1509,7 +1519,7 @@ void sendDataFrame(DataFrame frame) {
publisher.enqueue(encodeFrame(frame));
publisher.signalEnqueued();
} catch (IOException e) {
if (!closed) {
if (!isMarked(closedState, SHUTDOWN_REQUESTED)) {
Log.logError(e);
shutdown(e);
}
Expand All @@ -1527,7 +1537,7 @@ void sendUnorderedFrame(Http2Frame frame) {
publisher.enqueueUnordered(encodeFrame(frame));
publisher.signalEnqueued();
} catch (IOException e) {
if (!closed) {
if (!isMarked(closedState, SHUTDOWN_REQUESTED)) {
Log.logError(e);
shutdown(e);
}
Expand Down Expand Up @@ -1693,4 +1703,60 @@ AbstractAsyncSSLConnection getConnection() {
return connection;
}
}

private boolean isMarked(int state, int mask) {
return (state & mask) == mask;
}

private boolean markShutdownRequested() {
return markClosedState(SHUTDOWN_REQUESTED);
}

private boolean markHalfClosedLocal() {
return markClosedState(HALF_CLOSED_LOCAL);
}

private boolean markHalfClosedLRemote() {
return markClosedState(HALF_CLOSED_REMOTE);
}

private boolean markClosedState(int flag) {
int state, desired;
do {
state = desired = closedState;
if ((state & flag) == flag) return false;
desired = state | flag;
} while (!CLOSED_STATE.compareAndSet(this, state, desired));
return true;
}

String describeClosedState(int state) {
if (state == 0) return "active";
String desc = null;
if (isMarked(state, SHUTDOWN_REQUESTED)) {
desc = "shutdown";
}
if (isMarked(state, HALF_CLOSED_LOCAL | HALF_CLOSED_REMOTE)) {
if (desc == null) return "closed";
else return desc + "+closed";
}
if (isMarked(state, HALF_CLOSED_LOCAL)) {
if (desc == null) return "half-closed-local";
else return desc + "+half-closed-local";
}
if (isMarked(state, HALF_CLOSED_REMOTE)) {
if (desc == null) return "half-closed-remote";
else return desc + "+half-closed-remote";
}
return "0x" + Integer.toString(state, 16);
}

private static final VarHandle CLOSED_STATE;
static {
try {
CLOSED_STATE = MethodHandles.lookup().findVarHandle(Http2Connection.class, "closedState", int.class);
} catch (Exception x) {
throw new ExceptionInInitializerError(x);
}
}
}
4 changes: 3 additions & 1 deletion test/jdk/java/net/httpclient/http2/NoBodyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@
* @bug 8087112
* @library /test/lib /test/jdk/java/net/httpclient/lib
* @build jdk.test.lib.net.SimpleSSLContext jdk.httpclient.test.lib.http2.Http2TestServer
* @run testng/othervm -Djdk.httpclient.HttpClient.log=ssl,requests,responses,errors NoBodyTest
* @run testng/othervm -Djdk.httpclient.HttpClient.log=ssl,requests,responses,errors
* -Djdk.internal.httpclient.debug=true
* NoBodyTest
*/

import java.io.IOException;
Expand Down