Re: Tagging 9.0.x and 8.5.x

[Rémy Maucherat]

On Mon, Jun 3, 2019 at 11:40 PM Mark Thomas  wrote:

> http://people.apache.org/~markt/patches/2019-06-03-h2-v1.patch


I'm testing extensively, and this patch
http://people.apache.org/~markt/patches/2019-06-03-h2-v2.patch looks good
to me.

Rémy

Re: Tagging 9.0.x and 8.5.x

[Rémy Maucherat]

On Mon, Jun 3, 2019 at 11:40 PM Mark Thomas  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

Re: Tagging 9.0.x and 8.5.x

[Mark Thomas]

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

Mark

PS Having had to get my head around the non-blocking changes - kudos.
Nice work.

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: Tagging 9.0.x and 8.5.x

[Rémy Maucherat]

On Mon, Jun 3, 2019 at 11:06 PM Mark Thomas  wrote:

> On 03/06/2019 21:45, Rémy Maucherat wrote:
> > On Mon, Jun 3, 2019 at 10:05 PM Mark Thomas  > > wrote:
> >
> > On 03/06/2019 19:54, Rémy Maucherat wrote:
> >
> > 
> >
> > > Ok, that's completely different ;) I tried to be careful with
> > that, but
> > > the algorithm changes to non blocking, so obviously there were some
> > > risks involved. I'll have a quick look at it, but you can go ahead
> > with
> > > your window update fix and tag, this NIO2 problem is likely not a
> > super
> > > critical issue.
> >
> > I think I am making progress. The issue appears to stem from the
> > connection preface being processed in-line on Linux and OSX but not
> > in-line on Windows.
> >
> > I'm probably missing something because my fix so far (and it isn't
> > complete) is making changes to the Http2UpgradeHandler API.
> >
> >
> > Ok, that's very consistent with your concurrency issue. The problem may
> > be with the upgradeHandler.upgradeDispatch(SocketEvent.OPEN_READ); at
> > line 124 in Http2AsyncParser, which shouldn't be called in some cases.
> > I'm trying to experiment there first, but since I don't get the problem
> > it's hard to figure it out.
> > Making changes to Http2UpgradeHandler could be ok since the preface was
> > blocking and I already made some changes to accommodate async
> > (processConnection -> processConnectionCallback).
>
> 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.

Rémy

Re: Tagging 9.0.x and 8.5.x

[Mark Thomas]

On 03/06/2019 21:45, Rémy Maucherat wrote:
> On Mon, Jun 3, 2019 at 10:05 PM Mark Thomas  > wrote:
> 
> On 03/06/2019 19:54, Rémy Maucherat wrote:
> 
> 
> 
> > Ok, that's completely different ;) I tried to be careful with
> that, but
> > the algorithm changes to non blocking, so obviously there were some
> > risks involved. I'll have a quick look at it, but you can go ahead
> with
> > your window update fix and tag, this NIO2 problem is likely not a
> super
> > critical issue.
> 
> I think I am making progress. The issue appears to stem from the
> connection preface being processed in-line on Linux and OSX but not
> in-line on Windows.
> 
> I'm probably missing something because my fix so far (and it isn't
> complete) is making changes to the Http2UpgradeHandler API.
> 
> 
> Ok, that's very consistent with your concurrency issue. The problem may
> be with the upgradeHandler.upgradeDispatch(SocketEvent.OPEN_READ); at
> line 124 in Http2AsyncParser, which shouldn't be called in some cases.
> I'm trying to experiment there first, but since I don't get the problem
> it's hard to figure it out.
> Making changes to Http2UpgradeHandler could be ok since the preface was
> blocking and I already made some changes to accommodate async
> (processConnection -> processConnectionCallback).

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.

Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: Tagging 9.0.x and 8.5.x

[Rémy Maucherat]

On Mon, Jun 3, 2019 at 10:05 PM Mark Thomas  wrote:

> On 03/06/2019 19:54, Rémy Maucherat wrote:
>
> 
>
> > Ok, that's completely different ;) I tried to be careful with that, but
> > the algorithm changes to non blocking, so obviously there were some
> > risks involved. I'll have a quick look at it, but you can go ahead with
> > your window update fix and tag, this NIO2 problem is likely not a super
> > critical issue.
>
> I think I am making progress. The issue appears to stem from the
> connection preface being processed in-line on Linux and OSX but not
> in-line on Windows.
>
> I'm probably missing something because my fix so far (and it isn't
> complete) is making changes to the Http2UpgradeHandler API.
>

Ok, that's very consistent with your concurrency issue. The problem may be
with the upgradeHandler.upgradeDispatch(SocketEvent.OPEN_READ); at line 124
in Http2AsyncParser, which shouldn't be called in some cases. I'm trying to
experiment there first, but since I don't get the problem it's hard to
figure it out.
Making changes to Http2UpgradeHandler could be ok since the preface was
blocking and I already made some changes to accommodate async
(processConnection -> processConnectionCallback).

Rémy

Re: Tagging 9.0.x and 8.5.x

[Mark Thomas]

On 03/06/2019 19:54, Rémy Maucherat wrote:



> Ok, that's completely different ;) I tried to be careful with that, but
> the algorithm changes to non blocking, so obviously there were some
> risks involved. I'll have a quick look at it, but you can go ahead with
> your window update fix and tag, this NIO2 problem is likely not a super
> critical issue.

I think I am making progress. The issue appears to stem from the
connection preface being processed in-line on Linux and OSX but not
in-line on Windows.

I'm probably missing something because my fix so far (and it isn't
complete) is making changes to the Http2UpgradeHandler API.

Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: Tagging 9.0.x and 8.5.x

[Rémy Maucherat]

On Mon, Jun 3, 2019 at 8:36 PM Mark Thomas  wrote:

> On 03/06/2019 16:43, Mark Thomas wrote:
> > On 03/06/2019 14:15, Rémy Maucherat wrote:
>
> 
>
> >> I think I have found at least one more edge case around the
> >> Stream/Connection allocation handling. Fixing it is going to mean
> going
> >> back to a synchronizing on a single object (Stream) so the current
> >> notify Stream/Connection code is going to need some re-work.
> >>
> >> I have a potential patch but I haven't finished testing it yet. It
> also
> >> needs proper debug logging, i18n etc. If all goes well, I should be
> able
> >> to commit the fix and then tag later today. I'll post an update if
> >> things don't go well.
> >
> > Things seem to be improving. Linux and OSX are OK but NIO2 on Windows is
> > still failing some tests but fewer than before. I'm currently re-running
> > the unit tests with debug logging enabled to shed some light on what is
> > going wrong. As far as I can tell the issue(s) are still around handling
> > of WINDOW_UPDATE frames when the Connection/Stream flow control window
> > is exhausted.
>
> Further investigation has identified a different root cause. NIO2 is
> getting itself into a state where there are two threads processing the
> incoming frames and they are conflicting. I'm still trying to figure out
> the sequence of events that leads up to this state but it appears to be
> related to the processing of the connection preface.
>

Ok, that's completely different ;) I tried to be careful with that, but the
algorithm changes to non blocking, so obviously there were some risks
involved. I'll have a quick look at it, but you can go ahead with your
window update fix and tag, this NIO2 problem is likely not a super critical
issue.

Rémy

Re: Tagging 9.0.x and 8.5.x

[Mark Thomas]

On 03/06/2019 16:43, Mark Thomas wrote:
> On 03/06/2019 14:15, Rémy Maucherat wrote:



>> I think I have found at least one more edge case around the
>> Stream/Connection allocation handling. Fixing it is going to mean going
>> back to a synchronizing on a single object (Stream) so the current
>> notify Stream/Connection code is going to need some re-work.
>>
>> I have a potential patch but I haven't finished testing it yet. It also
>> needs proper debug logging, i18n etc. If all goes well, I should be able
>> to commit the fix and then tag later today. I'll post an update if
>> things don't go well.
> 
> Things seem to be improving. Linux and OSX are OK but NIO2 on Windows is
> still failing some tests but fewer than before. I'm currently re-running
> the unit tests with debug logging enabled to shed some light on what is
> going wrong. As far as I can tell the issue(s) are still around handling
> of WINDOW_UPDATE frames when the Connection/Stream flow control window
> is exhausted.

Further investigation has identified a different root cause. NIO2 is
getting itself into a state where there are two threads processing the
incoming frames and they are conflicting. I'm still trying to figure out
the sequence of events that leads up to this state but it appears to be
related to the processing of the connection preface.

Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: Tagging 9.0.x and 8.5.x

[Mark Thomas]

On 03/06/2019 14:15, Rémy Maucherat wrote:
> On Mon, Jun 3, 2019 at 1:21 PM Mark Thomas  > wrote:
> 
> Hi,
> 
> It is the start of another month so I intend to tag 9.0.x and 8.5.x
> shortly. Or at least I did until I found a handful of HTTP/2 related
> failures when I ran the unit tests on Windows for 9.0.x.
> 
> 
> Bad news :( I haven't run into it.

Indeed.

> +1 for the tags.
> 
> 
> I think I have found at least one more edge case around the
> Stream/Connection allocation handling. Fixing it is going to mean going
> back to a synchronizing on a single object (Stream) so the current
> notify Stream/Connection code is going to need some re-work.
> 
> I have a potential patch but I haven't finished testing it yet. It also
> needs proper debug logging, i18n etc. If all goes well, I should be able
> to commit the fix and then tag later today. I'll post an update if
> things don't go well.

Things seem to be improving. Linux and OSX are OK but NIO2 on Windows is
still failing some tests but fewer than before. I'm currently re-running
the unit tests with debug logging enabled to shed some light on what is
going wrong. As far as I can tell the issue(s) are still around handling
of WINDOW_UPDATE frames when the Connection/Stream flow control window
is exhausted.

Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: Tagging 9.0.x and 8.5.x

[Rémy Maucherat]

On Mon, Jun 3, 2019 at 1:21 PM Mark Thomas  wrote:

> Hi,
>
> It is the start of another month so I intend to tag 9.0.x and 8.5.x
> shortly. Or at least I did until I found a handful of HTTP/2 related
> failures when I ran the unit tests on Windows for 9.0.x.
>

Bad news :( I haven't run into it.
+1 for the tags.

>
> I think I have found at least one more edge case around the
> Stream/Connection allocation handling. Fixing it is going to mean going
> back to a synchronizing on a single object (Stream) so the current
> notify Stream/Connection code is going to need some re-work.
>
> I have a potential patch but I haven't finished testing it yet. It also
> needs proper debug logging, i18n etc. If all goes well, I should be able
> to commit the fix and then tag later today. I'll post an update if
> things don't go well.
>

Ok.

Rémy

Re: Tagging 9.0.x and 8.5.x

[Rémy Maucherat]

On Wed, May 1, 2019 at 11:23 PM Mark Thomas  wrote:

> Hi,
>
> Just a heads up that I'm intended to tag these soon. Possibly tomorrow
> but certainly by the end of the week. I just have a few things I want to
> look at first.
>

Should I leave the async IO code enabled, or should it remain disabled by
default for this build ?

Rémy

Re: Tagging 9.0.x and 8.5.x

[Woonsan Ko]

Hi Mark,

Thanks for the heads up!
I'd like to fill in some missing Korean translations (~ 10 new items),
and push the exported strings in the master branch tonight.

Cheers,

Woonsan

On Wed, May 1, 2019 at 5:23 PM Mark Thomas  wrote:
>
> Hi,
>
> Just a heads up that I'm intended to tag these soon. Possibly tomorrow
> but certainly by the end of the week. I just have a few things I want to
> look at first.
>
> Mark
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: dev-h...@tomcat.apache.org
>

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: Tagging 9.0.x and 8.5.x

[Mark Thomas]

On 31/05/18 15:14, Rémy Maucherat wrote:
> On Thu, May 31, 2018 at 3:20 PM Mark Thomas  wrote:
> 
>> Hi all,
>>
>> June is almost upon us so I'm thinking about the next release round.
>>
>> I've got a couple of small tasks on my personal TODO list and there are
>> one or two BZ issues that could be resolved. I plan to complete these
>> tasks and then tag. Depending how things go, I currently anticipate
>> tagging late this week or early next.
>>
> 
> +1
> 
> Rémy

FYI I'm planning on holding off for a few days to pick up Native
1.2.next once released.

Mark



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: Tagging 9.0.x and 8.5.x

[Rémy Maucherat]

On Thu, May 31, 2018 at 3:20 PM Mark Thomas  wrote:

> Hi all,
>
> June is almost upon us so I'm thinking about the next release round.
>
> I've got a couple of small tasks on my personal TODO list and there are
> one or two BZ issues that could be resolved. I plan to complete these
> tasks and then tag. Depending how things go, I currently anticipate
> tagging late this week or early next.
>

+1

Rémy

Re: Tagging 9.0.x and 8.5.x

[Rémy Maucherat]

On Tue, Apr 3, 2018 at 4:46 PM, Mark Thomas  wrote:

> As we have just passed the start of the month I plan to fix any open
> bugs and then tag with a view to releasing end of this week / early next.
>

+1
On the BZ front, it looks pretty good.

Rémy

Re: Tagging 9.0.x and 8.5.x

[Mark Thomas]

On 08/01/18 09:31, Konstantin Kolinko wrote:
> 2018-01-04 23:42 GMT+03:00 Mark Thomas :
>> Hi all,
>>
>> It is the start of a new month and the open issue list looks to be clear
>> so I'm planning on tagging 9.0.x and 8.5.x early next week.
> 
> Is there a need for a new Tomcat-Native build for Windows,
> to update to OpenSSL 1.0.2n (released 2017-12-07).
> 
> Tomcat Native 1.2.16 (released 2017-11-20) is built with 1.0.2m,
> 
> https://www.openssl.org/news/newslog.html
> 
> Generally, I think that CVE-2017-3737 does not affect us, as I read it that it
> relies on an application ignoring a fatal error from a handshake and
> continuing to read data,
> and I think that Tomcat won't ignore a fatal handshake error.

I concur. I wasn't planning on a Tomcat-Native release.

Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: Tagging 9.0.x and 8.5.x

[Konstantin Kolinko]

2018-01-04 23:42 GMT+03:00 Mark Thomas :
> Hi all,
>
> It is the start of a new month and the open issue list looks to be clear
> so I'm planning on tagging 9.0.x and 8.5.x early next week.

Is there a need for a new Tomcat-Native build for Windows,
to update to OpenSSL 1.0.2n (released 2017-12-07).

Tomcat Native 1.2.16 (released 2017-11-20) is built with 1.0.2m,

https://www.openssl.org/news/newslog.html

Generally, I think that CVE-2017-3737 does not affect us, as I read it that it
relies on an application ignoring a fatal error from a handshake and
continuing to read data,
and I think that Tomcat won't ignore a fatal handshake error.


Best regards,
Konstantin Kolinko

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org