On 04/06/2019 09:58, r...@apache.org wrote:
> This is an automated email from the ASF dual-hosted git repository.
>
> remm 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 a98f6d0 Fix rare ISE issue I see in TestAsyncFlush.NIO
> a98f6d0 is described below
>
> commit a98f6d02bb92ded2b3b0cb5438fb7c8adb8d85f9
> Author: remm
> AuthorDate: Tue Jun 4 10:58:24 2019 +0200
>
> Fix rare ISE issue I see in TestAsyncFlush.NIO
>
> It looks possibly ok to me to wait on both allocation types with
> waitForNonBlocking (that's what the test runs into for me, both reach 0
> at the same time).
That is definitely not OK. It should never happen.
The sequence is:
- Request allocation from stream. In none available, wait until some is.
- Request allocation from connection. In none available, wait until some
is.
- Then write.
It should be impossible for a Stream to be waiting for an allocation
from the Stream and the Connection.
Mark
> Also use NONE instead of 0.
> Tests pass for me, so trying with CI.
> ---
> .../coyote/http2/WindowAllocationManager.java | 34
> +++---
> 1 file changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/java/org/apache/coyote/http2/WindowAllocationManager.java
> b/java/org/apache/coyote/http2/WindowAllocationManager.java
> index 7626bf3..6510312 100644
> --- a/java/org/apache/coyote/http2/WindowAllocationManager.java
> +++ b/java/org/apache/coyote/http2/WindowAllocationManager.java
> @@ -121,7 +121,7 @@ class WindowAllocationManager {
>
> private void waitFor(int waitTarget, long timeout) throws
> InterruptedException {
> synchronized (stream) {
> -if (waitingFor != 0) {
> +if (waitingFor != NONE) {
> throw new
> IllegalStateException(sm.getString("windowAllocationManager.waitFor.ise",
> stream.getConnectionId(), stream.getIdentifier()));
> }
> @@ -134,23 +134,21 @@ class WindowAllocationManager {
> stream.wait(timeout);
> }
>
> -waitingFor = 0;
> +waitingFor = NONE;
> }
> }
>
>
> private void waitForNonBlocking(int waitTarget) {
> synchronized (stream) {
> -if (waitingFor == 0) {
> +if (waitingFor == NONE) {
> waitingFor = waitTarget;
> } else if (waitingFor == waitTarget) {
> // NO-OP
> // Non-blocking post-processing may attempt to flush
> } else {
> -throw new
> IllegalStateException(sm.getString("windowAllocationManager.waitFor.ise",
> -stream.getConnectionId(), stream.getIdentifier()));
> +waitingFor |= waitTarget;
> }
> -
> }
> }
>
> @@ -162,7 +160,7 @@ class WindowAllocationManager {
> }
>
> synchronized (stream) {
> -if ((notifyTarget & waitingFor) > 0) {
> +if ((notifyTarget & waitingFor) > NONE) {
> if (stream.getCoyoteResponse().getWriteListener() == null) {
> // Blocking, so use notify to release StreamOutputBuffer
> if (log.isDebugEnabled()) {
> @@ -171,17 +169,19 @@ class WindowAllocationManager {
> }
> stream.notify();
> } else {
> -waitingFor = 0;
> -// Non-blocking so dispatch
> -if (log.isDebugEnabled()) {
> -
> log.debug(sm.getString("windowAllocationManager.dispatched",
> -stream.getConnectionId(),
> stream.getIdentifier()));
> +waitingFor &= ~notifyTarget;
> +if (waitingFor == NONE) {
> +// Non-blocking so dispatch
> +if (log.isDebugEnabled()) {
> +
> log.debug(sm.getString("windowAllocationManager.dispatched",
> +stream.getConnectionId(),
> stream.getIdentifier()));
> +}
> +
> stream.getCoyoteResponse().action(ActionCode.DISPATCH_WRITE, null);
> +// Need to explicitly execute dispatches on the
> StreamProcessor
> +// as this thread is being processed by an
> UpgradeProcessor
> +// which won't see this dispatch
> +
> stream.getCoyoteResponse().action(ActionCode.DISPATCH_EXECUTE, null);
> }
> -
> stream.getCoyoteResponse().action(ActionCode.DISPATCH_WRITE, null);
> -// Need to explicitly execute dispatches on the
> StreamProcessor
> -// as this thread is being
