Re: [tomcat] branch master updated: Add a currently disabled test for async timeout with HTTP/2

2019-12-10 Thread Rémy Maucherat 
On Tue, Dec 10, 2019 at 9:06 AM Mark Thomas  wrote:

> On 09/12/2019 21:45, Rémy Maucherat wrote:
> > On Mon, Dec 9, 2019 at 7:59 PM Mark Thomas  > > wrote:
> >
> > On 09/12/2019 17:34, ma...@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 dcc6ad5  Add a currently disabled test for async timeout
> > with HTTP/2
> > > dcc6ad5 is described below
> > >
> > > commit dcc6ad52ad6a3ca53ebc1270b89df9fbc6da2f31
> > > Author: Mark Thomas mailto:ma...@apache.org>>
> > > AuthorDate: Mon Dec 9 17:34:10 2019 +
> > >
> > > Add a currently disabled test for async timeout with HTTP/2
> >
> > Note: This test isn't quite right yet. I'll commit the fix once I've
> > fixed the underlying issue. The issue is proving trickier than I
> thought
> > it would - mainly because it looks like I'm going to need to make a
> > bigger change than I really want to. I know what needs to happen, I'm
> > trying to figure out the best way to make it happen.
> >
> > There's indeed a problem and it's a lot harder than I expected.
> > With async the timeoutAsync goes nowhere as no processor gets added to
> > the waitingProcessors.
> > Without async, the timeout goes to the processor of the connection which
> > isn't much better, it would have to go to the stream which was doing the
> > async request (but it's not processed through the protocol, obviously).
> > Either way it's not going to work :(
>
> After looking at various options I opted to add a reference to the
> Http11Protocol in the Http2Protocol and then added the SocketProcessor
> to the waiting processors.
>
> I have a patch but I need to finish cleaning up the test so it runs
> cleanly.
>

Don't know about your plan there, but I would use an async thread at the
end of the test, otherwise I'm not sure the timeout will be processed:
asyncContext.setTimeout(2000);
new Thread(new Runnable() {
@Override
public void run() {
try {
Thread.sleep(1);
} catch (InterruptedException e) {
// Ignore
}
asyncContext.complete();
}
}).run();


>
> This has the added benefit that we can reassess the decision to repeat a
> lot of the Http11Procotol config on the Http2Protocol. We could, if we
> wished, just use the settings from the Http11Protocol. Something to
> consider for Tomcat 10 maybe. I'll ask on the users list and see how
> much interest there is in separate settings.
>

That could be a good move.


>
> > Earlier, I wanted to remove the waitingProcessors from AbstractProtocol
> > in favor of some processing in the endpoint. I found out that isn't a
> > very good idea.
> > Maybe instead the actual async timeout processing should be moved one
> > level up (do them in the Catalina adapter, but as usual use a dispatch
> > to do it when they happen). Would that work better ?
>
> I'm not sure. First impression is that it would mean quite a few
> changes. The timeout handling is currently internal to Coyote, this
> would make it part of the Coyote/Catalina API. That doesn't seem quite
> right to me at the moment.
>

No problem, my justification was that the async timeout is not a real IO
timeout but a timeout of the Servlet API, so that's why it could be in
Catalina.

Rémy

Re: [tomcat] branch master updated: Add a currently disabled test for async timeout with HTTP/2

2019-12-10 Thread Mark Thomas 
On 09/12/2019 21:45, Rémy Maucherat wrote:
> On Mon, Dec 9, 2019 at 7:59 PM Mark Thomas  > wrote:
> 
> On 09/12/2019 17:34, ma...@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 dcc6ad5  Add a currently disabled test for async timeout
> with HTTP/2
> > dcc6ad5 is described below
> >
> > commit dcc6ad52ad6a3ca53ebc1270b89df9fbc6da2f31
> > Author: Mark Thomas mailto:ma...@apache.org>>
> > AuthorDate: Mon Dec 9 17:34:10 2019 +
> >
> >     Add a currently disabled test for async timeout with HTTP/2
> 
> Note: This test isn't quite right yet. I'll commit the fix once I've
> fixed the underlying issue. The issue is proving trickier than I thought
> it would - mainly because it looks like I'm going to need to make a
> bigger change than I really want to. I know what needs to happen, I'm
> trying to figure out the best way to make it happen.
> 
> There's indeed a problem and it's a lot harder than I expected.
> With async the timeoutAsync goes nowhere as no processor gets added to
> the waitingProcessors.
> Without async, the timeout goes to the processor of the connection which
> isn't much better, it would have to go to the stream which was doing the
> async request (but it's not processed through the protocol, obviously).
> Either way it's not going to work :(

After looking at various options I opted to add a reference to the
Http11Protocol in the Http2Protocol and then added the SocketProcessor
to the waiting processors.

I have a patch but I need to finish cleaning up the test so it runs cleanly.

This has the added benefit that we can reassess the decision to repeat a
lot of the Http11Procotol config on the Http2Protocol. We could, if we
wished, just use the settings from the Http11Protocol. Something to
consider for Tomcat 10 maybe. I'll ask on the users list and see how
much interest there is in separate settings.

> Earlier, I wanted to remove the waitingProcessors from AbstractProtocol
> in favor of some processing in the endpoint. I found out that isn't a
> very good idea.
> Maybe instead the actual async timeout processing should be moved one
> level up (do them in the Catalina adapter, but as usual use a dispatch
> to do it when they happen). Would that work better ?

I'm not sure. First impression is that it would mean quite a few
changes. The timeout handling is currently internal to Coyote, this
would make it part of the Coyote/Catalina API. That doesn't seem quite
right to me at the moment.

Mark

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

Re: [tomcat] branch master updated: Add a currently disabled test for async timeout with HTTP/2

2019-12-09 Thread Rémy Maucherat 
On Mon, Dec 9, 2019 at 7:59 PM Mark Thomas  wrote:

> On 09/12/2019 17:34, ma...@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 dcc6ad5  Add a currently disabled test for async timeout with
> HTTP/2
> > dcc6ad5 is described below
> >
> > commit dcc6ad52ad6a3ca53ebc1270b89df9fbc6da2f31
> > Author: Mark Thomas 
> > AuthorDate: Mon Dec 9 17:34:10 2019 +
> >
> > Add a currently disabled test for async timeout with HTTP/2
>
> Note: This test isn't quite right yet. I'll commit the fix once I've
> fixed the underlying issue. The issue is proving trickier than I thought
> it would - mainly because it looks like I'm going to need to make a
> bigger change than I really want to. I know what needs to happen, I'm
> trying to figure out the best way to make it happen.
>
> There's indeed a problem and it's a lot harder than I expected.
With async the timeoutAsync goes nowhere as no processor gets added to the
waitingProcessors.
Without async, the timeout goes to the processor of the connection which
isn't much better, it would have to go to the stream which was doing the
async request (but it's not processed through the protocol, obviously).
Either way it's not going to work :(

Earlier, I wanted to remove the waitingProcessors from AbstractProtocol in
favor of some processing in the endpoint. I found out that isn't a very
good idea.
Maybe instead the actual async timeout processing should be moved one level
up (do them in the Catalina adapter, but as usual use a dispatch to do it
when they happen). Would that work better ?

Rémy

Re: [tomcat] branch master updated: Add a currently disabled test for async timeout with HTTP/2

2019-12-09 Thread Mark Thomas 
On 09/12/2019 17:34, ma...@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 dcc6ad5  Add a currently disabled test for async timeout with HTTP/2
> dcc6ad5 is described below
> 
> commit dcc6ad52ad6a3ca53ebc1270b89df9fbc6da2f31
> Author: Mark Thomas 
> AuthorDate: Mon Dec 9 17:34:10 2019 +
> 
> Add a currently disabled test for async timeout with HTTP/2

Note: This test isn't quite right yet. I'll commit the fix once I've
fixed the underlying issue. The issue is proving trickier than I thought
it would - mainly because it looks like I'm going to need to make a
bigger change than I really want to. I know what needs to happen, I'm
trying to figure out the best way to make it happen.

Mark


> ---
>  test/org/apache/coyote/http2/TestAsyncTimeout.java | 142 
> +
>  1 file changed, 142 insertions(+)
> 
> diff --git a/test/org/apache/coyote/http2/TestAsyncTimeout.java 
> b/test/org/apache/coyote/http2/TestAsyncTimeout.java
> new file mode 100644
> index 000..70a5348
> --- /dev/null
> +++ b/test/org/apache/coyote/http2/TestAsyncTimeout.java
> @@ -0,0 +1,142 @@
> +/*
> + *  Licensed to the Apache Software Foundation (ASF) under one or more
> + *  contributor license agreements.  See the NOTICE file distributed with
> + *  this work for additional information regarding copyright ownership.
> + *  The ASF licenses this file to You under the Apache License, Version 2.0
> + *  (the "License"); you may not use this file except in compliance with
> + *  the License.  You may obtain a copy of the License at
> + *
> + *  http://www.apache.org/licenses/LICENSE-2.0
> + *
> + *  Unless required by applicable law or agreed to in writing, software
> + *  distributed under the License is distributed on an "AS IS" BASIS,
> + *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + *  See the License for the specific language governing permissions and
> + *  limitations under the License.
> + */
> +package org.apache.coyote.http2;
> +
> +import java.io.IOException;
> +import java.io.PrintWriter;
> +import java.nio.ByteBuffer;
> +import java.util.concurrent.atomic.AtomicBoolean;
> +
> +import javax.servlet.AsyncContext;
> +import javax.servlet.AsyncEvent;
> +import javax.servlet.AsyncListener;
> +import javax.servlet.http.HttpServlet;
> +import javax.servlet.http.HttpServletRequest;
> +import javax.servlet.http.HttpServletResponse;
> +
> +import org.junit.Assert;
> +import org.junit.Ignore;
> +import org.junit.Test;
> +
> +import org.apache.catalina.Context;
> +import org.apache.catalina.Wrapper;
> +import org.apache.catalina.startup.Tomcat;
> +
> +public class TestAsyncTimeout extends Http2TestBase {
> +
> +@Ignore // Until this HTTP/2 + async timeouts is fixed
> +@Test
> +public void testTimeout() throws Exception {
> +enableHttp2();
> +
> +Tomcat tomcat = getTomcatInstance();
> +
> +Context ctxt = tomcat.addContext("", null);
> +Tomcat.addServlet(ctxt, "simple", new SimpleServlet());
> +ctxt.addServletMappingDecoded("/simple", "simple");
> +Wrapper w = Tomcat.addServlet(ctxt, "async", new 
> AsyncTimeoutServlet());
> +w.setAsyncSupported(true);
> +ctxt.addServletMappingDecoded("/async", "async");
> +tomcat.start();
> +
> +openClientConnection();
> +doHttpUpgrade();
> +sendClientPreface();
> +validateHttp2InitialResponse();
> +
> +// Reset connection window size after intial response
> +sendWindowUpdate(0, SimpleServlet.CONTENT_LENGTH);
> +
> +output.setTraceBody(true);
> +
> +// Send request
> +byte[] frameHeader = new byte[9];
> +ByteBuffer headersPayload = ByteBuffer.allocate(128);
> +buildGetRequest(frameHeader, headersPayload, null, 3, "/async");
> +writeFrame(frameHeader, headersPayload);
> +
> +// Headers
> +parser.readFrame(true);
> +// Body
> +parser.readFrame(true);
> +
> +// Check that the right number of bytes were received
> +String trace = output.getTrace();
> +Assert.assertTrue(trace, trace.contains("PASS"));
> +}
> +
> +
> +public static class AsyncTimeoutServlet extends HttpServlet {
> +
> +private static final long serialVersionUID = 1L;
> +
> +@Override
> +protected void doGet(HttpServletRequest request, HttpServletResponse 
> response)
> +throws IOException {
> +
> +final AsyncContext asyncContext = request.startAsync();
> +
> +response.setStatus(HttpServletResponse.SC_OK);
> +response.setContentType("text/plain");
> +response.setCharacterEncoding("UTF-8");
> +
> +

[tomcat] branch master updated: Add a currently disabled test for async timeout with HTTP/2

2019-12-09 Thread markt 
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 dcc6ad5  Add a currently disabled test for async timeout with HTTP/2
dcc6ad5 is described below

commit dcc6ad52ad6a3ca53ebc1270b89df9fbc6da2f31
Author: Mark Thomas 
AuthorDate: Mon Dec 9 17:34:10 2019 +

Add a currently disabled test for async timeout with HTTP/2
---
 test/org/apache/coyote/http2/TestAsyncTimeout.java | 142 +
 1 file changed, 142 insertions(+)

diff --git a/test/org/apache/coyote/http2/TestAsyncTimeout.java 
b/test/org/apache/coyote/http2/TestAsyncTimeout.java
new file mode 100644
index 000..70a5348
--- /dev/null
+++ b/test/org/apache/coyote/http2/TestAsyncTimeout.java
@@ -0,0 +1,142 @@
+/*
+ *  Licensed to the Apache Software Foundation (ASF) under one or more
+ *  contributor license agreements.  See the NOTICE file distributed with
+ *  this work for additional information regarding copyright ownership.
+ *  The ASF licenses this file to You under the Apache License, Version 2.0
+ *  (the "License"); you may not use this file except in compliance with
+ *  the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ */
+package org.apache.coyote.http2;
+
+import java.io.IOException;
+import java.io.PrintWriter;
+import java.nio.ByteBuffer;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import javax.servlet.AsyncContext;
+import javax.servlet.AsyncEvent;
+import javax.servlet.AsyncListener;
+import javax.servlet.http.HttpServlet;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import org.junit.Assert;
+import org.junit.Ignore;
+import org.junit.Test;
+
+import org.apache.catalina.Context;
+import org.apache.catalina.Wrapper;
+import org.apache.catalina.startup.Tomcat;
+
+public class TestAsyncTimeout extends Http2TestBase {
+
+@Ignore // Until this HTTP/2 + async timeouts is fixed
+@Test
+public void testTimeout() throws Exception {
+enableHttp2();
+
+Tomcat tomcat = getTomcatInstance();
+
+Context ctxt = tomcat.addContext("", null);
+Tomcat.addServlet(ctxt, "simple", new SimpleServlet());
+ctxt.addServletMappingDecoded("/simple", "simple");
+Wrapper w = Tomcat.addServlet(ctxt, "async", new 
AsyncTimeoutServlet());
+w.setAsyncSupported(true);
+ctxt.addServletMappingDecoded("/async", "async");
+tomcat.start();
+
+openClientConnection();
+doHttpUpgrade();
+sendClientPreface();
+validateHttp2InitialResponse();
+
+// Reset connection window size after intial response
+sendWindowUpdate(0, SimpleServlet.CONTENT_LENGTH);
+
+output.setTraceBody(true);
+
+// Send request
+byte[] frameHeader = new byte[9];
+ByteBuffer headersPayload = ByteBuffer.allocate(128);
+buildGetRequest(frameHeader, headersPayload, null, 3, "/async");
+writeFrame(frameHeader, headersPayload);
+
+// Headers
+parser.readFrame(true);
+// Body
+parser.readFrame(true);
+
+// Check that the right number of bytes were received
+String trace = output.getTrace();
+Assert.assertTrue(trace, trace.contains("PASS"));
+}
+
+
+public static class AsyncTimeoutServlet extends HttpServlet {
+
+private static final long serialVersionUID = 1L;
+
+@Override
+protected void doGet(HttpServletRequest request, HttpServletResponse 
response)
+throws IOException {
+
+final AsyncContext asyncContext = request.startAsync();
+
+response.setStatus(HttpServletResponse.SC_OK);
+response.setContentType("text/plain");
+response.setCharacterEncoding("UTF-8");
+
+asyncContext.addListener(new AsyncListener() {
+
+AtomicBoolean ended = new AtomicBoolean(false);
+
+@Override
+public void onTimeout(AsyncEvent event) throws IOException {
+if (ended.compareAndSet(false, true)) {
+PrintWriter pw = 
event.getAsyncContext().getResponse().getWriter();
+pw.write("PASS");
+pw.flush();
+event.getAsyncContext().complete();
+}
+}
+
+@Override
+public void onStartAsync(AsyncEvent event) throws