On Mon, Jun 3, 2019 at 11:40 PM Mark Thomas <ma...@apache.org> wrote:
> On 03/06/2019 22:29, Rémy Maucherat wrote:
>
> > How about this as an idea:
> >
> > --- a/java/org/apache/coyote/AbstractProtocol.java
> > +++ b/java/org/apache/coyote/AbstractProtocol.java
> > @@ -905,6 +905,10 @@
> > }
> > }
> > }
> > + // The handler will initiate any further I/O
> > + if (wrapper.hasAsyncIO()) {
> > + state = SocketState.LONG;
> > + }
> > }
> > } while ( state == SocketState.UPGRADING);
> >
> >
> > Essentially, it is saying if the upgrade handler is async, it will
> take
> > care of triggering any further reads that may be necessary.
> >
> > Initial test results are promising.
> >
> > Worth trying.
>
> Close, but it wasn't quite right. It is the UpgradeHandler that needs to
> be tested.
>
> This works with Linux. Just running the tests on Windows...
>
> http://people.apache.org/~markt/patches/2019-06-03-h2-v1.patch
Ok, it might produce good results.
This preface problem might be what made the sync on socketWrapper useful in
Http2AsyncParser L246 (with the patch below). I'll revisit it eventually,
since I don't really know why it became useful, it obviously used to work
without it. The non blocking preface reading is needed though (IMO) so it's
not really possible to go back on that.
I was trying this patch too, but I don't know if it fixes something:
diff --git a/java/org/apache/coyote/http2/Http2AsyncParser.java
b/java/org/apache/coyote/http2/Http2AsyncParser.java
index 55d97eb..337175b 100644
--- a/java/org/apache/coyote/http2/Http2AsyncParser.java
+++ b/java/org/apache/coyote/http2/Http2AsyncParser.java
@@ -120,8 +120,11 @@
}
// Finish processing the connection
upgradeHandler.processConnectionCallback(webConnection,
stream);
- // Continue reading frames
- upgradeHandler.upgradeDispatch(SocketEvent.OPEN_READ);
+ if (state == CompletionState.DONE) {
+ // The call was not completed inline, so must start
reading new frames
+ // or process the stream exception
+ upgradeHandler.upgradeDispatch(SocketEvent.OPEN_READ);
+ }
}
}
@@ -167,7 +170,7 @@
private boolean parsedFrameHeader = false;
private boolean validated = false;
- private CompletionState state = null;
+ protected CompletionState state = null;
protected int payloadSize;
protected FrameType frameType;
protected int flags;
>
>
> Mark
>
> PS Having had to get my head around the non-blocking changes - kudos.
> Nice work.
>
Thanks, but it's still broken, so it's not so good :(
Rémy
