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/01/08 14:11:00 UTC
svn commit: r1650280 -
/tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
Author: markt
Date: Thu Jan 8 13:10:59 2015
New Revision: 1650280
URL: http://svn.apache.org/r1650280
Log:
Fix failing unit test
testMessagesBlocking(org.apache.coyote.http11.upgrade.TestUpgrade)
Modified:
tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
Modified: tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java?rev=1650280&r1=1650279&r2=1650280&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java Thu Jan 8 13:10:59 2015
@@ -1099,9 +1099,7 @@ public class Nio2Endpoint extends Abstra
int thisTime = transfer(buf, off, len, socketWriteBuffer);
len = len - thisTime;
off = off + thisTime;
- if (socketWriteBuffer.remaining() == 0) {
- flush(true);
- }
+ flush(true);
}
} else {
// FIXME: Possible new behavior:
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1650280 - /tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
Posted by Rémy Maucherat <re...@apache.org>.
2015-01-08 15:48 GMT+01:00 Mark Thomas <ma...@apache.org>:
> The reason I only saw the issue with NIO2 is that only NIO2 was
> buffering the ServletOutputStream for upgraded connections. The patch
> above aligned NIO2 with NIO and APR - neither of which buffer upgraded
> connections.
>
> I took a look at the spec and it wasn't completely clear if the
> ServletOutputStream in an upgraded connection should be buffered or not.
>
> Against buffering is that all of the control is on the response - which
> isn't available for an upgraded connection.
>
> For buffering is the performance benefits and that nowhere does it say
> that ServletOutputStream is not buffered for upgrade.
>
> I can see the benefits of buffering here but given the typical use of
> upgraded connections I wonder if it isn't better handled at the
> application layer as and when required. Maybe something to clarify with
> the Servlet EG?
>
> Let's assume it's fine then.
Rémy
Re: svn commit: r1650280 - /tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
Posted by Mark Thomas <ma...@apache.org>.
On 08/01/2015 14:06, Mark Thomas wrote:
> On 08/01/2015 13:50, Rémy Maucherat wrote:
>> 2015-01-08 14:11 GMT+01:00 <ma...@apache.org>:
>>
>>> Author: markt
>>> Date: Thu Jan 8 13:10:59 2015
>>> New Revision: 1650280
>>>
>>> URL: http://svn.apache.org/r1650280
>>> Log:
>>> Fix failing unit test
>>> testMessagesBlocking(org.apache.coyote.http11.upgrade.TestUpgrade)
>>>
>>> Modified:
>>> tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
>>>
>>> Modified: tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
>>> URL:
>>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java?rev=1650280&r1=1650279&r2=1650280&view=diff
>>>
>>> ==============================================================================
>>> --- tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
>>> (original)
>>> +++ tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java Thu
>>> Jan 8 13:10:59 2015
>>> @@ -1099,9 +1099,7 @@ public class Nio2Endpoint extends Abstra
>>> int thisTime = transfer(buf, off, len,
>>> socketWriteBuffer);
>>> len = len - thisTime;
>>> off = off + thisTime;
>>> - if (socketWriteBuffer.remaining() == 0) {
>>> - flush(true);
>>> - }
>>> + flush(true);
>>> }
>>>
>> This doesn't look very good. Removing buffering = worse performance.
>
> Agreed.
>
>> This is probably because upgrade wouldn't deal with buffering as the message
>> implies, so this could need an extra flag.
>
> On taking another look the problem is further up the call stack - the
> patch above was just working around it. I need to look into why I only
> saw the issue with NIO2.
The reason I only saw the issue with NIO2 is that only NIO2 was
buffering the ServletOutputStream for upgraded connections. The patch
above aligned NIO2 with NIO and APR - neither of which buffer upgraded
connections.
I took a look at the spec and it wasn't completely clear if the
ServletOutputStream in an upgraded connection should be buffered or not.
Against buffering is that all of the control is on the response - which
isn't available for an upgraded connection.
For buffering is the performance benefits and that nowhere does it say
that ServletOutputStream is not buffered for upgrade.
I can see the benefits of buffering here but given the typical use of
upgraded connections I wonder if it isn't better handled at the
application layer as and when required. Maybe something to clarify with
the Servlet EG?
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1650280 - /tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
Posted by Mark Thomas <ma...@apache.org>.
On 08/01/2015 13:50, Rémy Maucherat wrote:
> 2015-01-08 14:11 GMT+01:00 <ma...@apache.org>:
>
>> Author: markt
>> Date: Thu Jan 8 13:10:59 2015
>> New Revision: 1650280
>>
>> URL: http://svn.apache.org/r1650280
>> Log:
>> Fix failing unit test
>> testMessagesBlocking(org.apache.coyote.http11.upgrade.TestUpgrade)
>>
>> Modified:
>> tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
>>
>> Modified: tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
>> URL:
>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java?rev=1650280&r1=1650279&r2=1650280&view=diff
>>
>> ==============================================================================
>> --- tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
>> (original)
>> +++ tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java Thu
>> Jan 8 13:10:59 2015
>> @@ -1099,9 +1099,7 @@ public class Nio2Endpoint extends Abstra
>> int thisTime = transfer(buf, off, len,
>> socketWriteBuffer);
>> len = len - thisTime;
>> off = off + thisTime;
>> - if (socketWriteBuffer.remaining() == 0) {
>> - flush(true);
>> - }
>> + flush(true);
>> }
>>
> This doesn't look very good. Removing buffering = worse performance.
Agreed.
> This is probably because upgrade wouldn't deal with buffering as the message
> implies, so this could need an extra flag.
On taking another look the problem is further up the call stack - the
patch above was just working around it. I need to look into why I only
saw the issue with NIO2.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1650280 - /tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
Posted by Rémy Maucherat <re...@apache.org>.
2015-01-08 14:11 GMT+01:00 <ma...@apache.org>:
> Author: markt
> Date: Thu Jan 8 13:10:59 2015
> New Revision: 1650280
>
> URL: http://svn.apache.org/r1650280
> Log:
> Fix failing unit test
> testMessagesBlocking(org.apache.coyote.http11.upgrade.TestUpgrade)
>
> Modified:
> tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
>
> Modified: tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
> URL:
> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java?rev=1650280&r1=1650279&r2=1650280&view=diff
>
> ==============================================================================
> --- tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java
> (original)
> +++ tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java Thu
> Jan 8 13:10:59 2015
> @@ -1099,9 +1099,7 @@ public class Nio2Endpoint extends Abstra
> int thisTime = transfer(buf, off, len,
> socketWriteBuffer);
> len = len - thisTime;
> off = off + thisTime;
> - if (socketWriteBuffer.remaining() == 0) {
> - flush(true);
> - }
> + flush(true);
> }
>
> This doesn't look very good. Removing buffering = worse performance. This
is probably because upgrade wouldn't deal with buffering as the message
implies, so this could need an extra flag.
Rémy