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/03/09 17:32:43 UTC
svn commit: r1665297 -
/tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
Author: markt
Date: Mon Mar 9 16:32:43 2015
New Revision: 1665297
URL: http://svn.apache.org/r1665297
Log:
Follow up to r1665062
Callers assume that output() will fully write the data that is passed to it. Ensure that this is the case when the AJP message size is larger than the output buffer.
Modified:
tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
Modified: tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java?rev=1665297&r1=1665296&r2=1665297&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java (original)
+++ tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java Mon Mar 9 16:32:43 2015
@@ -302,24 +302,29 @@ public class AjpNioProcessor extends Abs
ByteBuffer writeBuffer =
socketWrapper.getSocket().getBufHandler().getWriteBuffer();
- int toWrite = Math.min(length, writeBuffer.remaining());
- writeBuffer.put(src, offset, toWrite);
-
- writeBuffer.flip();
-
- long writeTimeout = att.getWriteTimeout();
- Selector selector = null;
- try {
- selector = pool.get();
- } catch ( IOException x ) {
- //ignore
- }
- try {
- pool.write(writeBuffer, socketWrapper.getSocket(), selector,
- writeTimeout, true);
- }finally {
- writeBuffer.clear();
- if ( selector != null ) pool.put(selector);
+ int left = length;
+ int written = 0;
+ while (left > 0) {
+ int toWrite = Math.min(left, writeBuffer.remaining());
+ writeBuffer.put(src, offset, toWrite);
+
+ writeBuffer.flip();
+
+ long writeTimeout = att.getWriteTimeout();
+ Selector selector = null;
+ try {
+ selector = pool.get();
+ } catch ( IOException x ) {
+ //ignore
+ }
+ try {
+ written = pool.write(writeBuffer, socketWrapper.getSocket(),
+ selector, writeTimeout, true);
+ } finally {
+ writeBuffer.clear();
+ if ( selector != null ) pool.put(selector);
+ }
+ left -= written;
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1665297 -
/tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 3/9/15 4:26 PM, Mark Thomas wrote:
> On 09/03/2015 18:34, Christopher Schultz wrote:
>> Mark,
>
>>> Glad to see it is working now. The next jobs are a) documenting the
>>> expected behavior re full vs partial write and b) take a look at trying
>>> to reduce the number of array copies (or equivalent) that the I/O codes
>>> does.
>>
>> I'm not sure it's really worth it to fix, at least not for Tomcat 7/8.
>> I'd focus on Tomcat 9 where you guys have been doing good work
>
> Agreed that is the focus. I haven't really formed a firm opinion on
> what, if anything, should be back-ported.
>
>
>> (except
>> for removing BIO! Boo! Hiss! ... yeah, I know why and I agree but I'm
>> still not terribly happy about it).
>
> As I have said before, if you can come up with a BIO implementation that
> works with the new API, I wouldn't object to adding BIO support back.
I think you and I both know that if you can't conceive of such an
implementation with more than 3 seconds of thought, I don't have a
prayer of designing and implementing it for quite some time.
:)
-chris
Re: svn commit: r1665297 - /tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
Posted by Mark Thomas <ma...@apache.org>.
On 09/03/2015 18:34, Christopher Schultz wrote:
> Mark,
>> Glad to see it is working now. The next jobs are a) documenting the
>> expected behavior re full vs partial write and b) take a look at trying
>> to reduce the number of array copies (or equivalent) that the I/O codes
>> does.
>
> I'm not sure it's really worth it to fix, at least not for Tomcat 7/8.
> I'd focus on Tomcat 9 where you guys have been doing good work
Agreed that is the focus. I haven't really formed a firm opinion on
what, if anything, should be back-ported.
> (except
> for removing BIO! Boo! Hiss! ... yeah, I know why and I agree but I'm
> still not terribly happy about it).
As I have said before, if you can come up with a BIO implementation that
works with the new API, I wouldn't object to adding BIO support back.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1665297 -
/tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 3/9/15 1:23 PM, Mark Thomas wrote:
> On 9 March 2015 16:35:57 GMT+00:00, Christopher Schultz <ch...@christopherschultz.net> wrote:
>> Mark,
>>
>> On 3/9/15 12:32 PM, markt@apache.org wrote:
>>> Author: markt
>>> Date: Mon Mar 9 16:32:43 2015
>>> New Revision: 1665297
>>>
>>> URL: http://svn.apache.org/r1665297
>>> Log:
>>> Follow up to r1665062
>>> Callers assume that output() will fully write the data that is passed
>> to it. Ensure that this is the case when the AJP message size is larger
>> than the output buffer.
>>>
>>> Modified:
>>>
>> tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
>>>
>>> Modified:
>> tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
>>> URL:
>> http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java?rev=1665297&r1=1665296&r2=1665297&view=diff
>>>
>> ==============================================================================
>>> ---
>> tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
>> (original)
>>> +++
>> tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
>> Mon Mar 9 16:32:43 2015
>>> @@ -302,24 +302,29 @@ public class AjpNioProcessor extends Abs
>>> ByteBuffer writeBuffer =
>>>
>> socketWrapper.getSocket().getBufHandler().getWriteBuffer();
>>>
>>> - int toWrite = Math.min(length, writeBuffer.remaining());
>>> - writeBuffer.put(src, offset, toWrite);
>>> -
>>> - writeBuffer.flip();
>>> -
>>> - long writeTimeout = att.getWriteTimeout();
>>> - Selector selector = null;
>>> - try {
>>> - selector = pool.get();
>>> - } catch ( IOException x ) {
>>> - //ignore
>>> - }
>>> - try {
>>> - pool.write(writeBuffer, socketWrapper.getSocket(),
>> selector,
>>> - writeTimeout, true);
>>> - }finally {
>>> - writeBuffer.clear();
>>> - if ( selector != null ) pool.put(selector);
>>> + int left = length;
>>> + int written = 0;
>>> + while (left > 0) {
>>> + int toWrite = Math.min(left, writeBuffer.remaining());
>>> + writeBuffer.put(src, offset, toWrite);
>>> +
>>> + writeBuffer.flip();
>>> +
>>> + long writeTimeout = att.getWriteTimeout();
>>> + Selector selector = null;
>>> + try {
>>> + selector = pool.get();
>>> + } catch ( IOException x ) {
>>> + //ignore
>>> + }
>>> + try {
>>> + written = pool.write(writeBuffer,
>> socketWrapper.getSocket(),
>>> + selector, writeTimeout, true);
>>> + } finally {
>>> + writeBuffer.clear();
>>> + if ( selector != null ) pool.put(selector);
>>> + }
>>> + left -= written;
>>> }
>>> }
>>
>> Okay, yes, this was more like what I was expecting to see. I'll give it
>> another try.
>
> Glad to see it is working now. The next jobs are a) documenting the
> expected behavior re full vs partial write and b) take a look at trying
> to reduce the number of array copies (or equivalent) that the I/O codes
> does.
I'm not sure it's really worth it to fix, at least not for Tomcat 7/8.
I'd focus on Tomcat 9 where you guys have been doing good work (except
for removing BIO! Boo! Hiss! ... yeah, I know why and I agree but I'm
still not terribly happy about it).
Definitely reducing otherwise useless copying is good, but don't kill
yourself over it in stable versions unless there is a specific
complaint. There are too many opportunities for breakage. I'm actually
shocked that nobody found this problem before. I guess not that many
people use mod_jk, or maybe most of them use it with the default packet
size.
I only found this because of two events that occurred very far apart in
time:
1a. I was doing testing on client certificate handling ... years ago.
Client certificates tend to grow large and since the whole request
headers must fit into a single AJP packet, I had to increase
max_packet_size in mod_jk. I did this for my "template" worker, to
affect all workers that derive from it.
1b. I changed my "client certificate test" application to use a matching
max packet size to 1a.
2. Recent use of GWT had started sending rather large payloads back and
forth to our servers. (Yes, I'm also curious why GWT, supposedly to
improve performance, is now sending more data to and from the server
than just refreshing the page... but where was I?) This notified us that
the packetSize in Tomcat was incorrect.
Adjusting the packetSize in Tomcat immediately caused all hell to break
loose.
Well, at any rate, I'm glad it's fixed, now.
Thanks,
-chris
Re: svn commit: r1665297 - /tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
Posted by Mark Thomas <ma...@homeinbox.net>.
On 9 March 2015 16:35:57 GMT+00:00, Christopher Schultz <ch...@christopherschultz.net> wrote:
>Mark,
>
>On 3/9/15 12:32 PM, markt@apache.org wrote:
>> Author: markt
>> Date: Mon Mar 9 16:32:43 2015
>> New Revision: 1665297
>>
>> URL: http://svn.apache.org/r1665297
>> Log:
>> Follow up to r1665062
>> Callers assume that output() will fully write the data that is passed
>to it. Ensure that this is the case when the AJP message size is larger
>than the output buffer.
>>
>> Modified:
>>
>tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
>>
>> Modified:
>tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
>> URL:
>http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java?rev=1665297&r1=1665296&r2=1665297&view=diff
>>
>==============================================================================
>> ---
>tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
>(original)
>> +++
>tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
>Mon Mar 9 16:32:43 2015
>> @@ -302,24 +302,29 @@ public class AjpNioProcessor extends Abs
>> ByteBuffer writeBuffer =
>>
>socketWrapper.getSocket().getBufHandler().getWriteBuffer();
>>
>> - int toWrite = Math.min(length, writeBuffer.remaining());
>> - writeBuffer.put(src, offset, toWrite);
>> -
>> - writeBuffer.flip();
>> -
>> - long writeTimeout = att.getWriteTimeout();
>> - Selector selector = null;
>> - try {
>> - selector = pool.get();
>> - } catch ( IOException x ) {
>> - //ignore
>> - }
>> - try {
>> - pool.write(writeBuffer, socketWrapper.getSocket(),
>selector,
>> - writeTimeout, true);
>> - }finally {
>> - writeBuffer.clear();
>> - if ( selector != null ) pool.put(selector);
>> + int left = length;
>> + int written = 0;
>> + while (left > 0) {
>> + int toWrite = Math.min(left, writeBuffer.remaining());
>> + writeBuffer.put(src, offset, toWrite);
>> +
>> + writeBuffer.flip();
>> +
>> + long writeTimeout = att.getWriteTimeout();
>> + Selector selector = null;
>> + try {
>> + selector = pool.get();
>> + } catch ( IOException x ) {
>> + //ignore
>> + }
>> + try {
>> + written = pool.write(writeBuffer,
>socketWrapper.getSocket(),
>> + selector, writeTimeout, true);
>> + } finally {
>> + writeBuffer.clear();
>> + if ( selector != null ) pool.put(selector);
>> + }
>> + left -= written;
>> }
>> }
>
>Okay, yes, this was more like what I was expecting to see. I'll give it
>another try.
Glad to see it is working now. The next jobs are a) documenting the expected behavior re full vs partial write and b) take a look at trying to reduce the number of array copies (or equivalent) that the I/O codes does.
Mark
>
>Thanks!
>-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1665297 -
/tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 3/9/15 12:32 PM, markt@apache.org wrote:
> Author: markt
> Date: Mon Mar 9 16:32:43 2015
> New Revision: 1665297
>
> URL: http://svn.apache.org/r1665297
> Log:
> Follow up to r1665062
> Callers assume that output() will fully write the data that is passed to it. Ensure that this is the case when the AJP message size is larger than the output buffer.
>
> Modified:
> tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
>
> Modified: tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
> URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java?rev=1665297&r1=1665296&r2=1665297&view=diff
> ==============================================================================
> --- tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java (original)
> +++ tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java Mon Mar 9 16:32:43 2015
> @@ -302,24 +302,29 @@ public class AjpNioProcessor extends Abs
> ByteBuffer writeBuffer =
> socketWrapper.getSocket().getBufHandler().getWriteBuffer();
>
> - int toWrite = Math.min(length, writeBuffer.remaining());
> - writeBuffer.put(src, offset, toWrite);
> -
> - writeBuffer.flip();
> -
> - long writeTimeout = att.getWriteTimeout();
> - Selector selector = null;
> - try {
> - selector = pool.get();
> - } catch ( IOException x ) {
> - //ignore
> - }
> - try {
> - pool.write(writeBuffer, socketWrapper.getSocket(), selector,
> - writeTimeout, true);
> - }finally {
> - writeBuffer.clear();
> - if ( selector != null ) pool.put(selector);
> + int left = length;
> + int written = 0;
> + while (left > 0) {
> + int toWrite = Math.min(left, writeBuffer.remaining());
> + writeBuffer.put(src, offset, toWrite);
> +
> + writeBuffer.flip();
> +
> + long writeTimeout = att.getWriteTimeout();
> + Selector selector = null;
> + try {
> + selector = pool.get();
> + } catch ( IOException x ) {
> + //ignore
> + }
> + try {
> + written = pool.write(writeBuffer, socketWrapper.getSocket(),
> + selector, writeTimeout, true);
> + } finally {
> + writeBuffer.clear();
> + if ( selector != null ) pool.put(selector);
> + }
> + left -= written;
> }
> }
Okay, yes, this was more like what I was expecting to see. I'll give it
another try.
Thanks!
-chris