On Tue, Dec 10, 2019 at 9:06 AM Mark Thomas <ma...@apache.org> wrote:
> On 09/12/2019 21:45, Rémy Maucherat wrote: > > On Mon, Dec 9, 2019 at 7:59 PM Mark Thomas <ma...@apache.org > > <mailto:ma...@apache.org>> wrote: > > > > On 09/12/2019 17:34, ma...@apache.org <mailto: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 <ma...@apache.org <mailto:ma...@apache.org>> > > > AuthorDate: Mon Dec 9 17:34:10 2019 +0000 > > > > > > 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(10000); } 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
