tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ma...@apache.org
Subject [tomcat] branch 8.5.x updated: Fix concurrency issue that caused intermittent h2 test failures
Date Mon, 13 May 2019 10:13:54 GMT
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 <markt@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


Mime
View raw message