You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2015/05/26 19:20:13 UTC
svn commit: r1681794 -
/tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Protocol.java
Author: markt
Date: Tue May 26 17:20:12 2015
New Revision: 1681794
URL: http://svn.apache.org/r1681794
Log:
Fix NIO2 test failures.
Modified:
tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Protocol.java
Modified: tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Protocol.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Protocol.java?rev=1681794&r1=1681793&r2=1681794&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Protocol.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Protocol.java Tue May 26 17:20:12 2015
@@ -22,7 +22,6 @@ import org.apache.juli.logging.LogFactor
import org.apache.tomcat.util.net.Nio2Channel;
import org.apache.tomcat.util.net.Nio2Endpoint;
import org.apache.tomcat.util.net.Nio2Endpoint.Handler;
-import org.apache.tomcat.util.net.Nio2Endpoint.Nio2SocketWrapper;
import org.apache.tomcat.util.net.SocketWrapperBase;
@@ -109,9 +108,8 @@ public class Http11Nio2Protocol extends
if (socket.isAsync()) {
((Nio2Endpoint) getProtocol().getEndpoint()).removeTimeout(socket);
}
- if (addToPoller) {
- ((Nio2SocketWrapper) socket).awaitBytes();
- }
+ // No need to add to poller. read() will have already been called
+ // with an appropriate completion handler.
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1681794 - /tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Protocol.java
Posted by Mark Thomas <ma...@apache.org>.
On 27/05/2015 08:30, Rémy Maucherat wrote:
> 2015-05-27 8:44 GMT+02:00 Mark Thomas <ma...@apache.org>:
>
>> On 26/05/2015 19:43, Rémy Maucherat wrote:
>>> 2015-05-26 20:37 GMT+02:00 Mark Thomas <ma...@apache.org>:
>>>
>>>> On 26/05/2015 19:33, Rémy Maucherat wrote:
>>>>> 2015-05-26 20:17 GMT+02:00 Mark Thomas <ma...@apache.org>:
>>>>>
>>>>>> It was a left over from the attempt to make BIO support more
>> connections
>>>>>> than it had threads by not doing an immediate read on a keep-alive
>>>>>> connection. It was no longer required. There is another "return false"
>>>>>> in the correct place in the next if block.
>>>>>>
>>>>>> The testsuite still fails with NIO2, so something has to be wrong :)
>>>>
>>>> It hasn't had a chance to run with my fix yet.
>>>>
>>> That is correct, I thought the previous one had it already.
>>
>> Now I do see some test failures with my fix in place. I'll take a look
>> at these today.
>>
> You're running into my little tricks which aimed at avoiding that a
> processor with a pending read would get disassociated from its socket, and
> thus it wasn't a good idea to try a non blocking read with NIO2 since it
> causes side effects. Since you moved the IO code into the socket, this is
> no longer a problem in theory.
That matches up with what I am seeing. I also see how the Async tests
are failing.
I'm not yet sure on the best way to fix this. I'm currently looking at
different options.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1681794 - /tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Protocol.java
Posted by Rémy Maucherat <re...@apache.org>.
2015-05-27 8:44 GMT+02:00 Mark Thomas <ma...@apache.org>:
> On 26/05/2015 19:43, Rémy Maucherat wrote:
> > 2015-05-26 20:37 GMT+02:00 Mark Thomas <ma...@apache.org>:
> >
> >> On 26/05/2015 19:33, Rémy Maucherat wrote:
> >>> 2015-05-26 20:17 GMT+02:00 Mark Thomas <ma...@apache.org>:
> >>>
> >>>> It was a left over from the attempt to make BIO support more
> connections
> >>>> than it had threads by not doing an immediate read on a keep-alive
> >>>> connection. It was no longer required. There is another "return false"
> >>>> in the correct place in the next if block.
> >>>>
> >>>> The testsuite still fails with NIO2, so something has to be wrong :)
> >>
> >> It hasn't had a chance to run with my fix yet.
> >>
> > That is correct, I thought the previous one had it already.
>
> Now I do see some test failures with my fix in place. I'll take a look
> at these today.
>
> You're running into my little tricks which aimed at avoiding that a
processor with a pending read would get disassociated from its socket, and
thus it wasn't a good idea to try a non blocking read with NIO2 since it
causes side effects. Since you moved the IO code into the socket, this is
no longer a problem in theory.
Rémy
Re: svn commit: r1681794 - /tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Protocol.java
Posted by Mark Thomas <ma...@apache.org>.
On 26/05/2015 19:43, Rémy Maucherat wrote:
> 2015-05-26 20:37 GMT+02:00 Mark Thomas <ma...@apache.org>:
>
>> On 26/05/2015 19:33, Rémy Maucherat wrote:
>>> 2015-05-26 20:17 GMT+02:00 Mark Thomas <ma...@apache.org>:
>>>
>>>> It was a left over from the attempt to make BIO support more connections
>>>> than it had threads by not doing an immediate read on a keep-alive
>>>> connection. It was no longer required. There is another "return false"
>>>> in the correct place in the next if block.
>>>>
>>>> The testsuite still fails with NIO2, so something has to be wrong :)
>>
>> It hasn't had a chance to run with my fix yet.
>>
> That is correct, I thought the previous one had it already.
Now I do see some test failures with my fix in place. I'll take a look
at these today.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1681794 - /tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Protocol.java
Posted by Rémy Maucherat <re...@apache.org>.
2015-05-26 20:37 GMT+02:00 Mark Thomas <ma...@apache.org>:
> On 26/05/2015 19:33, Rémy Maucherat wrote:
> > 2015-05-26 20:17 GMT+02:00 Mark Thomas <ma...@apache.org>:
> >
> >> It was a left over from the attempt to make BIO support more connections
> >> than it had threads by not doing an immediate read on a keep-alive
> >> connection. It was no longer required. There is another "return false"
> >> in the correct place in the next if block.
> >>
> >> The testsuite still fails with NIO2, so something has to be wrong :)
>
> It hasn't had a chance to run with my fix yet.
>
> That is correct, I thought the previous one had it already.
Rémy
Re: svn commit: r1681794 - /tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Protocol.java
Posted by Mark Thomas <ma...@apache.org>.
On 26/05/2015 19:33, Rémy Maucherat wrote:
> 2015-05-26 20:17 GMT+02:00 Mark Thomas <ma...@apache.org>:
>
>> It was a left over from the attempt to make BIO support more connections
>> than it had threads by not doing an immediate read on a keep-alive
>> connection. It was no longer required. There is another "return false"
>> in the correct place in the next if block.
>>
>> The testsuite still fails with NIO2, so something has to be wrong :)
It hasn't had a chance to run with my fix yet.
> Calling fill is not a noop, for starters.
No it isn't. The read may complete in-line and if it doesn't it the
completion handler will be called when it has completed.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1681794 - /tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Protocol.java
Posted by Rémy Maucherat <re...@apache.org>.
2015-05-26 20:17 GMT+02:00 Mark Thomas <ma...@apache.org>:
> It was a left over from the attempt to make BIO support more connections
> than it had threads by not doing an immediate read on a keep-alive
> connection. It was no longer required. There is another "return false"
> in the correct place in the next if block.
>
> The testsuite still fails with NIO2, so something has to be wrong :)
Calling fill is not a noop, for starters.
Rémy
Re: svn commit: r1681794 - /tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Protocol.java
Posted by Mark Thomas <ma...@apache.org>.
On 26/05/2015 18:41, Rémy Maucherat wrote:
<snip/>
My understanding of your response was that you are now happy with that
change. Shout if that is not the case.
> As far as I am concerned, the tweaking of the request line looks wrong.
>
> if (pos >= lastValid) {
> - if (useAvailableDataOnly) {
> - return false;
> + if (keptAlive) {
> + // Haven't read any request data yet so use the
> keep-alive
> + // timeout.
> +
> wrapper.setReadTimeout(wrapper.getEndpoint().getKeepAliveTimeout());
> }
>
> Ok to change the timeout if you like (although for NIO2 it's meaningless if
> the operation is already pending),
The keepAliveTimeout is only changed in the case where:
- keep-alive is enabled
- there is no more data in the read buffer
- a non-blocking read is required for the first byte (or more) of the
next request
In all other cases the soTimeout is used.
> but more importantly why is the "return false" gone ?
It was a left over from the attempt to make BIO support more connections
than it had threads by not doing an immediate read on a keep-alive
connection. It was no longer required. There is another "return false"
in the correct place in the next if block.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1681794 - /tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Protocol.java
Posted by Rémy Maucherat <re...@apache.org>.
2015-05-26 19:32 GMT+02:00 Mark Thomas <ma...@apache.org>:
> On 26/05/2015 18:27, Rémy Maucherat wrote:
> > 2015-05-26 19:20 GMT+02:00 <ma...@apache.org>:
> >
> >> Author: markt
> >> Date: Tue May 26 17:20:12 2015
> >> New Revision: 1681794
> >>
> >> URL: http://svn.apache.org/r1681794
> >> Log:
> >> Fix NIO2 test failures.
> >>
> > I don't understand what these failures suddenly come from, but this is
> > likely wrong: it is possible there is no pending read.
>
> The failures are timing related. I suspect they were always possible and
> just become more likely when I tweaked the code around non-blocking read
> of the request line.
>
> The problem was that there were two reads in progress. One from the read
> triggered by the non-blocking read of the request line and one from the
> awaitBytes() call triggered if the above non-blocking read returned zero.
>
> What are the scenarios that we do need to call awaitBytes() ?
>
So addToPoller needed to be true, that only happens for simple HTTP/1.1
keepalive. I don't see any guarantee there's a pending read really, so it
sounds logical to me.
As far as I am concerned, the tweaking of the request line looks wrong.
if (pos >= lastValid) {
- if (useAvailableDataOnly) {
- return false;
+ if (keptAlive) {
+ // Haven't read any request data yet so use the
keep-alive
+ // timeout.
+
wrapper.setReadTimeout(wrapper.getEndpoint().getKeepAliveTimeout());
}
Ok to change the timeout if you like (although for NIO2 it's meaningless if
the operation is already pending), but more importantly why is the "return
false" gone ?
Rémy
Re: svn commit: r1681794 - /tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Protocol.java
Posted by Mark Thomas <ma...@apache.org>.
On 26/05/2015 18:27, Rémy Maucherat wrote:
> 2015-05-26 19:20 GMT+02:00 <ma...@apache.org>:
>
>> Author: markt
>> Date: Tue May 26 17:20:12 2015
>> New Revision: 1681794
>>
>> URL: http://svn.apache.org/r1681794
>> Log:
>> Fix NIO2 test failures.
>>
> I don't understand what these failures suddenly come from, but this is
> likely wrong: it is possible there is no pending read.
The failures are timing related. I suspect they were always possible and
just become more likely when I tweaked the code around non-blocking read
of the request line.
The problem was that there were two reads in progress. One from the read
triggered by the non-blocking read of the request line and one from the
awaitBytes() call triggered if the above non-blocking read returned zero.
What are the scenarios that we do need to call awaitBytes() ?
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1681794 - /tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Protocol.java
Posted by Rémy Maucherat <re...@apache.org>.
2015-05-26 19:20 GMT+02:00 <ma...@apache.org>:
> Author: markt
> Date: Tue May 26 17:20:12 2015
> New Revision: 1681794
>
> URL: http://svn.apache.org/r1681794
> Log:
> Fix NIO2 test failures.
>
> I don't understand what these failures suddenly come from, but this is
likely wrong: it is possible there is no pending read.
Rémy