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/30 12:39:36 UTC
[tomcat] branch master updated: Additional h2 concurrency / timeout
fix
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/master by this push:
new 0bebe66 Additional h2 concurrency / timeout fix
0bebe66 is described below
commit 0bebe66d921072a3423cece63f3c2f8787d0342c
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Thu May 30 13:39:05 2019 +0100
Additional h2 concurrency / timeout fix
---
.../apache/coyote/http2/Http2UpgradeHandler.java | 37 ++++++++++++++++++----
1 file changed, 30 insertions(+), 7 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index 880d365..4ecb642 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -133,6 +133,24 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
private final PingManager pingManager = getPingManager();
private volatile int newStreamsSinceLastPrune = 0;
// Tracking for when the connection is blocked (windowSize < 1)
+ // The int array must have 3 elements. There are:
+ // [0] - The number of bytes the Stream requires from the connection
+ // window. This excludes any allocation that has already been made.
+ // [1] - The number of bytes that has been allocated from the connection
+ // window. This excludes any bytes that have been written since the
+ // allocation was made.
+ // [2] - 1 if the stream thread has been notified that an allocation has
+ // been made but has not yet consumed that allocation. 0 in all
+ // other cases. The purpose of this is to avoid the incorrect
+ // triggering of a timeout for the following sequence of events:
+ // window update 1
+ // allocation 1
+ // notify 1
+ // window update 2
+ // allocation 2
+ // act on notify 1 (using allocation 1 and 2)
+ // notify 2
+ // act on notify 2 (timeout due to no allocation)
private final Map<AbstractStream,int[]> backLogStreams = new ConcurrentHashMap<>();
private long backLogSize = 0;
@@ -765,12 +783,12 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
// Has this stream been granted an allocation
int[] value = backLogStreams.get(stream);
if (value == null) {
- value = new int[] { reservation, 0 };
+ value = new int[] { reservation, 0, 0 };
backLogStreams.put(stream, value);
backLogSize += reservation;
// Add the parents as well
AbstractStream parent = stream.getParentStream();
- while (parent != null && backLogStreams.putIfAbsent(parent, new int[2]) == null) {
+ while (parent != null && backLogStreams.putIfAbsent(parent, new int[3]) == null) {
parent = parent.getParentStream();
}
} else {
@@ -783,11 +801,13 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
// backlog.
backLogStreams.remove(stream);
} else {
- // This allocation has been used. Reset the
- // allocation to zero. Leave the stream on
- // the backlog as it still has more bytes to
- // write.
+ // This allocation has been used. Leave the
+ // stream on the backlog as it still has
+ // more bytes to write.
+ // Reset the allocation to zero.
value[1] = 0;
+ // Clear the notify in progress marker
+ value[2] = 0;
}
}
}
@@ -931,7 +951,10 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
int allocation = entry.getValue()[1];
if (allocation > 0) {
backLogSize -= allocation;
- result.add(entry.getKey());
+ if (entry.getValue()[2] == 0) {
+ result.add(entry.getKey());
+ entry.getValue()[2] = 1;
+ }
}
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch master updated: Additional h2 concurrency /
timeout fix
Posted by Mark Thomas <ma...@apache.org>.
> Mark,
>
> On 5/30/19 08:39, markt@apache.org wrote:
>> This is an automated email from the ASF dual-hosted git
>> repository.
>
>> markt pushed a commit to branch master in repository
>> https://gitbox.apache.org/repos/asf/tomcat.git
>
>
>> The following commit(s) were added to refs/heads/master by this
>> push: new 0bebe66 Additional h2 concurrency / timeout fix 0bebe66
>> is described below
>
>> commit 0bebe66d921072a3423cece63f3c2f8787d0342c Author: Mark Thomas
>> <ma...@apache.org> AuthorDate: Thu May 30 13:39:05 2019 +0100
>
>> Additional h2 concurrency / timeout fix ---
>> .../apache/coyote/http2/Http2UpgradeHandler.java | 37
>> ++++++++++++++++++---- 1 file changed, 30 insertions(+), 7
>> deletions(-)
>
>> diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
>> b/java/org/apache/coyote/http2/Http2UpgradeHandler.java index
>> 880d365..4ecb642 100644 ---
>> a/java/org/apache/coyote/http2/Http2UpgradeHandler.java +++
>> b/java/org/apache/coyote/http2/Http2UpgradeHandler.java @@ -133,6
>> +133,24 @@ class Http2UpgradeHandler extends AbstractStream
>> implements InternalHttpUpgradeH private final PingManager
>> pingManager = getPingManager(); private volatile int
>> newStreamsSinceLastPrune = 0; // Tracking for when the connection
>> is blocked (windowSize < 1) + // The int array must have 3
>> elements. There are: + // [0] - The number of bytes the Stream
>> requires from the connection + // window. This excludes
>> any allocation that has already been made. + // [1] - The
>> number of bytes that has been allocated from the connection + //
>> window. This excludes any bytes that have been written since the +
>> // allocation was made. + // [2] - 1 if the stream
>> thread has been notified that an allocation has + //
>> been made but has not yet consumed that allocation. 0 in all +
>> // other cases. The purpose of this is to avoid the
>> incorrect + // triggering of a timeout for the following
>> sequence of events: + // window update 1 + //
>> allocation 1 + // notify 1 + // window update
>> 2 + // allocation 2 + // act on notify 1
>> (using allocation 1 and 2) + // notify 2 + //
>> act on notify 2 (timeout due to no allocation)
>
> Maybe it's time for a real class instead of an int array that requires
> a half-page of documentation. We may still require the half-page of
> documentation, but using an int[] here seems like less and less of a
> good idea.
>
> Binding the documentation to a class would be more readable,
> especially in an IDE where you can get javadoc for a method / class
> easily without even opening the file.
Fair point. I'll take a look at that.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch master updated: Additional h2 concurrency /
timeout fix
Posted by Christopher Schultz <ch...@christopherschultz.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Mark,
On 5/30/19 08:39, markt@apache.org wrote:
> This is an automated email from the ASF dual-hosted git
> repository.
>
> markt pushed a commit to branch master in repository
> https://gitbox.apache.org/repos/asf/tomcat.git
>
>
> The following commit(s) were added to refs/heads/master by this
> push: new 0bebe66 Additional h2 concurrency / timeout fix 0bebe66
> is described below
>
> commit 0bebe66d921072a3423cece63f3c2f8787d0342c Author: Mark Thomas
> <ma...@apache.org> AuthorDate: Thu May 30 13:39:05 2019 +0100
>
> Additional h2 concurrency / timeout fix ---
> .../apache/coyote/http2/Http2UpgradeHandler.java | 37
> ++++++++++++++++++---- 1 file changed, 30 insertions(+), 7
> deletions(-)
>
> diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
> b/java/org/apache/coyote/http2/Http2UpgradeHandler.java index
> 880d365..4ecb642 100644 ---
> a/java/org/apache/coyote/http2/Http2UpgradeHandler.java +++
> b/java/org/apache/coyote/http2/Http2UpgradeHandler.java @@ -133,6
> +133,24 @@ class Http2UpgradeHandler extends AbstractStream
> implements InternalHttpUpgradeH private final PingManager
> pingManager = getPingManager(); private volatile int
> newStreamsSinceLastPrune = 0; // Tracking for when the connection
> is blocked (windowSize < 1) + // The int array must have 3
> elements. There are: + // [0] - The number of bytes the Stream
> requires from the connection + // window. This excludes
> any allocation that has already been made. + // [1] - The
> number of bytes that has been allocated from the connection + //
> window. This excludes any bytes that have been written since the +
> // allocation was made. + // [2] - 1 if the stream
> thread has been notified that an allocation has + //
> been made but has not yet consumed that allocation. 0 in all +
> // other cases. The purpose of this is to avoid the
> incorrect + // triggering of a timeout for the following
> sequence of events: + // window update 1 + //
> allocation 1 + // notify 1 + // window update
> 2 + // allocation 2 + // act on notify 1
> (using allocation 1 and 2) + // notify 2 + //
> act on notify 2 (timeout due to no allocation)
Maybe it's time for a real class instead of an int array that requires
a half-page of documentation. We may still require the half-page of
documentation, but using an int[] here seems like less and less of a
good idea.
Binding the documentation to a class would be more readable,
especially in an IDE where you can get javadoc for a method / class
easily without even opening the file.
- -chris
-----BEGIN PGP SIGNATURE-----
Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/
iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlzwHnQACgkQHPApP6U8
pFilmw//eEpyj/Qu649nCxI+k3pFdoCXNedUgONW+Yy7MbMCGNT1IUgAH0I6CBqM
j8YGcv+cvVTmnPJT0M+KtV+j0F+fBZhHsHo00NjGrcP6GEGItpihq1QyLecJ6+gJ
yLRujdOKj8ij9w9pRJ50XcHlksKEEUOaU/Ou1AMnZ0OlHXjHAB7zSRnqKvYMbVmJ
ZrD3nzlJYP70CSUJYYJZTpLAeWTwFe50xbqHl7mvp2H1iWRj+Zdqbs5hUEg6AyTC
K+tGHWKlvHYlneRSTSBCm1MvDKLRpgzwyrKRViiSGnCG4C2QETExPWSPM0Ylz/fW
X/7kIhxOXVQsfDkjyt0Zl+r/u1U18m1UIEEndaTW/uOq0yoeWWeqqfHt+28gmbLK
RKcejI4sfQKvC4HC207h/nyzmqz0SwerzJKxd7NuXIlhozQ+jYxw33iHoqZI7eYJ
zQBYEIp4YgpqkvO3mzJN8NX4LsN+mSRj/Gxy/qoVEPdKNT9UqgFpzKTfwa/1W5ib
rjdlbjZvuNn7bCq6UXHlU3ceBSnWoj/uCiK1RDIrNdSWfx/G6vRLREyI8DHh8bE6
bn79WhAIOLISmZPyJraFRxHcTAzb7nncYH0vnymPPXmZBV85jd2O58RCCmSqr5dA
OyAOk1hwFlo99+aXR+a4R8IhC7cdk/InDAvDRRQzOpTdAD0sisM=
=ZpwO
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org