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 <[email protected]>
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: [email protected]
For additional commands, e-mail: [email protected]
