svn commit: r1650280 - /tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java

2015-01-08 Thread markt 
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=1650280r1=1650279r2=1650280view=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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: svn commit: r1650280 - /tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java

2015-01-08 Thread Rémy Maucherat 
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=1650280r1=1650279r2=1650280view=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

Re: svn commit: r1650280 - /tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java

2015-01-08 Thread Mark Thomas 
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=1650280r1=1650279r2=1650280view=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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: svn commit: r1650280 - /tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java

2015-01-08 Thread Mark Thomas 
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=1650280r1=1650279r2=1650280view=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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: svn commit: r1650280 - /tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java

2015-01-08 Thread Rémy Maucherat 
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