You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2019/05/13 10:13:54 UTC
[tomcat] branch 8.5.x updated: Fix concurrency issue that caused
intermittent h2 test failures
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 8.5.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/8.5.x by this push:
new 50ea37e Fix concurrency issue that caused intermittent h2 test failures
50ea37e is described below
commit 50ea37ec4ccdabb615b53744c7c242970bb4ae7a
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Mon May 13 11:13:38 2019 +0100
Fix concurrency issue that caused intermittent h2 test failures
The Stream object was being used for both Stream window allocations and
connection window allocations. If a stream allocation occurred while
waiting for a connection allocation (and vice versa) processing would
continue on the incorrect assumption that the allocation being waited
for had occurred.
---
.../apache/coyote/http2/Http2UpgradeHandler.java | 60 ++++++++++++----------
.../apache/coyote/http2/LocalStrings.properties | 5 +-
java/org/apache/coyote/http2/Stream.java | 22 ++++++--
test/org/apache/coyote/http2/Http2TestBase.java | 10 ++--
webapps/docs/changelog.xml | 4 ++
5 files changed, 64 insertions(+), 37 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index a9ab68d..ffb0919 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -752,10 +752,11 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU
int reserveWindowSize(Stream stream, int reservation, boolean block) throws IOException {
- // Need to be holding the stream lock so releaseBacklog() can't notify
- // this thread until after this thread enters wait()
+ // Need to be holding the connection allocation lock so releaseBacklog()
+ // can't notify this thread until after this thread enters wait()
int allocation = 0;
- synchronized (stream) {
+ Object connectionAllocationLock = stream.getConnectionAllocationLock();
+ synchronized (connectionAllocationLock) {
do {
synchronized (this) {
if (!stream.canWrite()) {
@@ -810,22 +811,30 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU
// timeout
long writeTimeout = protocol.getWriteTimeout();
if (writeTimeout < 0) {
- stream.wait();
+ connectionAllocationLock.wait();
} else {
- stream.wait(writeTimeout);
- }
- // Has this stream been granted an allocation
- // Note: If the stream in not in this Map then the
- // requested write has been fully allocated
- int[] value = backLogStreams.get(stream);
- if (value != null && value[1] == 0) {
- // No allocation
- // Close the connection. Do this first since
- // closing the stream will raise an exception
- close();
- // Close the stream (in app code so need to
- // signal to app stream is closing)
- stream.doWriteTimeout();
+ connectionAllocationLock.wait(writeTimeout);
+ // Has this stream been granted an allocation
+ // Note: If the stream in not in this Map then the
+ // requested write has been fully allocated
+ int[] value;
+ // Ensure allocations made in other threads are visible
+ synchronized (this) {
+ value = backLogStreams.get(stream);
+ }
+ if (value != null && value[1] == 0) {
+ if (log.isDebugEnabled()) {
+ log.debug(sm.getString("upgradeHandler.noAllocation",
+ connectionId, stream.getIdentifier()));
+ }
+ // No allocation
+ // Close the connection. Do this first since
+ // closing the stream will raise an exception
+ close();
+ // Close the stream (in app code so need to
+ // signal to app stream is closing)
+ stream.doWriteTimeout();
+ }
}
} catch (InterruptedException e) {
throw new IOException(sm.getString(
@@ -843,7 +852,7 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU
- @SuppressWarnings("sync-override") // notifyAll() needs to be outside sync
+ @SuppressWarnings("sync-override") // notify() needs to be outside sync
// to avoid deadlock
@Override
protected void incrementWindowSize(int increment) throws Http2Exception {
@@ -872,12 +881,13 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU
Response coyoteResponse = ((Stream) stream).getCoyoteResponse();
if (coyoteResponse.getWriteListener() == null) {
if (log.isDebugEnabled()) {
- log.debug(sm.getString("upgradeHandler.notifyAll",
+ log.debug(sm.getString("upgradeHandler.notify",
connectionId, stream.getIdentifier()));
}
// Blocking, so use notify to release StreamOutputBuffer
- synchronized (stream) {
- stream.notifyAll();
+ Object connectionAllocationLock = ((Stream) stream).getConnectionAllocationLock();
+ synchronized (connectionAllocationLock) {
+ connectionAllocationLock.notify();
}
} else {
if (log.isDebugEnabled()) {
@@ -1052,12 +1062,8 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU
for (Stream stream : streams.values()) {
// The connection is closing. Close the associated streams as no
- // longer required.
+ // longer required (also notifies any threads waiting for allocations).
stream.receiveReset(Http2Error.CANCEL.getCode());
- // Release any streams waiting for an allocation
- synchronized (stream) {
- stream.notifyAll();
- }
}
try {
socketWrapper.close();
diff --git a/java/org/apache/coyote/http2/LocalStrings.properties b/java/org/apache/coyote/http2/LocalStrings.properties
index 24a7d1a..dc3bd1c 100644
--- a/java/org/apache/coyote/http2/LocalStrings.properties
+++ b/java/org/apache/coyote/http2/LocalStrings.properties
@@ -124,8 +124,9 @@ upgradeHandler.init=Connection [{0}], State [{1}]
upgradeHandler.initialWindowSize.invalid=Connection [{0}], Illegal value of [{1}] ignored for initial window size
upgradeHandler.invalidPreface=Connection [{0}], Invalid connection preface
upgradeHandler.ioerror=Connection [{0}]
+upgradeHandler.noAllocation=Connection [{0}], Stream [{1}], Timeout waiting for allocation
upgradeHandler.noNewStreams=Connection [{0}], Stream [{1}], Stream ignored as no new streams are permitted on this connection
-upgradeHandler.notifyAll=Connection [{0}], Stream [{1}], notifyAll() called to release StreamOutputBuffer
+upgradeHandler.notify=Connection [{0}], Stream [{1}], notify() called to release StreamOutputBuffer
upgradeHandler.pause.entry=Connection [{0}] Pausing
upgradeHandler.prefaceReceived=Connection [{0}], Connection preface received from client
upgradeHandler.pingFailed=Connection [{0}] Failed to send ping to client
@@ -156,4 +157,4 @@ upgradeHandler.writeHeaders=Connection [{0}], Stream [{1}]
upgradeHandler.writePushHeaders=Connection [{0}], Stream [{1}], Pushed stream [{2}], EndOfStream [{3}]
writeStateMachine.endWrite.ise=It is illegal to specify [{0}] for the new state once a write has completed
-writeStateMachine.ise=It is illegal to call [{0}()] in state [{1}]
\ No newline at end of file
+writeStateMachine.ise=It is illegal to call [{0}()] in state [{1}]
diff --git a/java/org/apache/coyote/http2/Stream.java b/java/org/apache/coyote/http2/Stream.java
index d7b2006..7f783fd 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -69,6 +69,8 @@ public class Stream extends AbstractStream implements HeaderEmitter {
private final Http2UpgradeHandler handler;
private final StreamStateMachine state;
+ private final Object connectionAllocationLock = new Object();
+
// State machine would be too much overhead
private int headerState = HEADER_STATE_START;
private StreamException headerException = null;
@@ -227,9 +229,18 @@ public class Stream extends AbstractStream implements HeaderEmitter {
if (inputBuffer != null) {
inputBuffer.receiveReset();
}
- // Writes wait on Stream so we can notify directly
+ cancelAllocationRequests();
+ }
+
+
+ final void cancelAllocationRequests() {
+ // Cancel wait on stream allocation request (if any)
synchronized (this) {
- this.notifyAll();
+ this.notify();
+ }
+ // Cancel wait on connection allocation request (if any)
+ synchronized (connectionAllocationLock) {
+ connectionAllocationLock.notify();
}
}
@@ -250,7 +261,7 @@ public class Stream extends AbstractStream implements HeaderEmitter {
if (notify && getWindowSize() > 0) {
if (coyoteResponse.getWriteListener() == null) {
// Blocking, so use notify to release StreamOutputBuffer
- notifyAll();
+ notify();
} else {
// Non-blocking so dispatch
coyoteResponse.action(ActionCode.DISPATCH_WRITE, null);
@@ -717,6 +728,11 @@ public class Stream extends AbstractStream implements HeaderEmitter {
}
+ Object getConnectionAllocationLock() {
+ return connectionAllocationLock;
+ }
+
+
private static void push(final Http2UpgradeHandler handler, final Request request,
final Stream stream) throws IOException {
if (org.apache.coyote.Constants.IS_SECURITY_ENABLED) {
diff --git a/test/org/apache/coyote/http2/Http2TestBase.java b/test/org/apache/coyote/http2/Http2TestBase.java
index 827be45..db5451b 100644
--- a/test/org/apache/coyote/http2/Http2TestBase.java
+++ b/test/org/apache/coyote/http2/Http2TestBase.java
@@ -535,11 +535,11 @@ public abstract class Http2TestBase extends TomcatBaseTest {
Connector connector = getTomcatInstance().getConnector();
Http2Protocol http2Protocol = new Http2Protocol();
// Short timeouts for now. May need to increase these for CI systems.
- http2Protocol.setReadTimeout(2000);
- http2Protocol.setWriteTimeout(2000);
- http2Protocol.setKeepAliveTimeout(5000);
- http2Protocol.setStreamReadTimeout(1000);
- http2Protocol.setStreamWriteTimeout(1000);
+ http2Protocol.setReadTimeout(6000);
+ http2Protocol.setWriteTimeout(6000);
+ http2Protocol.setKeepAliveTimeout(15000);
+ http2Protocol.setStreamReadTimeout(3000);
+ http2Protocol.setStreamWriteTimeout(3000);
http2Protocol.setMaxConcurrentStreams(maxConcurrentStreams);
connector.addUpgradeProtocol(http2Protocol);
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index eaba5dd..6d28e08 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -66,6 +66,10 @@
<bug>63412</bug>: Security manager failure when using the async IO
API from a webapp. (remm)
</fix>
+ <fix>
+ Fix concurrency issue that lead to incorrect HTTP/2 connection timeout.
+ (remm/markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Other">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org