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 2022/05/24 16:46:32 UTC
[tomcat] branch 9.0.x updated: Additional fixes for 66076
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/9.0.x by this push:
new 680db4448d Additional fixes for 66076
680db4448d is described below
commit 680db4448d392752fb57b91639e7ab34a3f58105
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Tue May 24 17:46:20 2022 +0100
Additional fixes for 66076
The vectored IO version of the fix.
---
java/org/apache/tomcat/util/net/AprEndpoint.java | 7 +++++++
java/org/apache/tomcat/util/net/Nio2Endpoint.java | 7 +++++++
java/org/apache/tomcat/util/net/NioEndpoint.java | 19 ++++++++++++-------
.../org/apache/tomcat/util/net/SocketWrapperBase.java | 4 +++-
4 files changed, 29 insertions(+), 8 deletions(-)
diff --git a/java/org/apache/tomcat/util/net/AprEndpoint.java b/java/org/apache/tomcat/util/net/AprEndpoint.java
index c02e90fc09..20d10efa11 100644
--- a/java/org/apache/tomcat/util/net/AprEndpoint.java
+++ b/java/org/apache/tomcat/util/net/AprEndpoint.java
@@ -2813,6 +2813,13 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
return inline;
}
+ @Override
+ protected boolean hasOutboundRemaining() {
+ // NIO2 never has remaining outbound data when the completion
+ // handler is called
+ return false;
+ }
+
@Override
public void run() {
// Perform the IO operation
diff --git a/java/org/apache/tomcat/util/net/Nio2Endpoint.java b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
index 49ee411016..c5b9a395f5 100644
--- a/java/org/apache/tomcat/util/net/Nio2Endpoint.java
+++ b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
@@ -1021,6 +1021,13 @@ public class Nio2Endpoint extends AbstractJsseEndpoint<Nio2Channel,AsynchronousS
return Nio2Endpoint.isInline();
}
+ @Override
+ protected boolean hasOutboundRemaining() {
+ // NIO2 never has remaining outbound data when the completion
+ // handler is called
+ return false;
+ }
+
@Override
protected void start() {
if (read) {
diff --git a/java/org/apache/tomcat/util/net/NioEndpoint.java b/java/org/apache/tomcat/util/net/NioEndpoint.java
index b8b6d4339d..ea65dd6600 100644
--- a/java/org/apache/tomcat/util/net/NioEndpoint.java
+++ b/java/org/apache/tomcat/util/net/NioEndpoint.java
@@ -1622,6 +1622,11 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel>
return inline;
}
+ @Override
+ protected boolean hasOutboundRemaining() {
+ return getSocket().getOutboundRemaining() > 0;
+ }
+
@Override
public void run() {
// Perform the IO operation
@@ -1654,13 +1659,13 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel>
} else {
boolean doWrite = true;
// Write from main buffer first
- if (!socketBufferHandler.isWriteBufferEmpty()) {
+ if (socketOrNetworkBufferHasDataLeft()) {
// There is still data inside the main write buffer, it needs to be written first
socketBufferHandler.configureWriteBufferForRead();
do {
nBytes = getSocket().write(socketBufferHandler.getWriteBuffer());
- } while (!socketBufferHandler.isWriteBufferEmpty() && nBytes > 0);
- if (!socketBufferHandler.isWriteBufferEmpty()) {
+ } while (socketOrNetworkBufferHasDataLeft() && nBytes > 0);
+ if (socketOrNetworkBufferHasDataLeft()) {
doWrite = false;
}
// Preserve a negative value since it is an error
@@ -1681,7 +1686,8 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel>
updateLastWrite();
}
}
- if (nBytes != 0 || !buffersArrayHasRemaining(buffers, offset, length)) {
+ if (nBytes != 0 || (!buffersArrayHasRemaining(buffers, offset, length) &&
+ !socketOrNetworkBufferHasDataLeft())) {
completionDone = false;
}
}
@@ -1689,7 +1695,8 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel>
setError(e);
}
}
- if (nBytes > 0 || (nBytes == 0 && !buffersArrayHasRemaining(buffers, offset, length))) {
+ if (nBytes > 0 || (nBytes == 0 && !buffersArrayHasRemaining(buffers, offset, length) &&
+ !socketOrNetworkBufferHasDataLeft())) {
// The bytes processed are only updated in the completion handler
completion.completed(Long.valueOf(nBytes), this);
} else if (nBytes < 0 || getError() != null) {
@@ -1708,9 +1715,7 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel>
}
}
}
-
}
-
}
diff --git a/java/org/apache/tomcat/util/net/SocketWrapperBase.java b/java/org/apache/tomcat/util/net/SocketWrapperBase.java
index bcb0119fc6..d95a31a15d 100644
--- a/java/org/apache/tomcat/util/net/SocketWrapperBase.java
+++ b/java/org/apache/tomcat/util/net/SocketWrapperBase.java
@@ -1036,6 +1036,8 @@ public abstract class SocketWrapperBase<E> {
*/
protected abstract boolean isInline();
+ protected abstract boolean hasOutboundRemaining();
+
/**
* Process the operation using the connector executor.
* @return true if the operation was accepted, false if the executor
@@ -1087,7 +1089,7 @@ public abstract class SocketWrapperBase<E> {
boolean completion = true;
if (state.check != null) {
CompletionHandlerCall call = state.check.callHandler(currentState, state.buffers, state.offset, state.length);
- if (call == CompletionHandlerCall.CONTINUE) {
+ if (call == CompletionHandlerCall.CONTINUE || state.hasOutboundRemaining()) {
complete = false;
} else if (call == CompletionHandlerCall.NONE) {
completion = false;
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch 9.0.x updated: Additional fixes for 66076
Posted by Mark Thomas <ma...@apache.org>.
On 25/05/2022 10:07, Rémy Maucherat wrote:
> On Tue, May 24, 2022 at 6:46 PM <ma...@apache.org> wrote:
>>
>> This is an automated email from the ASF dual-hosted git repository.
>>
>> markt pushed a commit to branch 9.0.x
>> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>>
>>
>> The following commit(s) were added to refs/heads/9.0.x by this push:
>> new 680db4448d Additional fixes for 66076
>> 680db4448d is described below
>>
>> commit 680db4448d392752fb57b91639e7ab34a3f58105
>> Author: Mark Thomas <ma...@apache.org>
>> AuthorDate: Tue May 24 17:46:20 2022 +0100
>>
>> Additional fixes for 66076
>>
>> The vectored IO version of the fix.
<snip/>
>> diff --git a/java/org/apache/tomcat/util/net/SocketWrapperBase.java b/java/org/apache/tomcat/util/net/SocketWrapperBase.java
>> index bcb0119fc6..d95a31a15d 100644
>> --- a/java/org/apache/tomcat/util/net/SocketWrapperBase.java
>> +++ b/java/org/apache/tomcat/util/net/SocketWrapperBase.java
>> @@ -1036,6 +1036,8 @@ public abstract class SocketWrapperBase<E> {
>> */
>> protected abstract boolean isInline();
>>
>> + protected abstract boolean hasOutboundRemaining();
>> +
>> /**
>> * Process the operation using the connector executor.
>> * @return true if the operation was accepted, false if the executor
>> @@ -1087,7 +1089,7 @@ public abstract class SocketWrapperBase<E> {
>> boolean completion = true;
>> if (state.check != null) {
>> CompletionHandlerCall call = state.check.callHandler(currentState, state.buffers, state.offset, state.length);
>> - if (call == CompletionHandlerCall.CONTINUE) {
>> + if (call == CompletionHandlerCall.CONTINUE || state.hasOutboundRemaining()) {
>
> I understand why it's there (the operation will indeed seem complete
> in that case, but it's not fully complete), but it needs to check
> state.read then (looping a read operation will most likely not do
> anything good; maybe it shouldn't happen, but it's a lot better to be
> safe), so:
> if (call == CompletionHandlerCall.CONTINUE || (!state.read &&
> state.hasOutboundRemaining())) {
>
> Overall the change is more convoluted than I expected, and I would
> have messed it up without a test case.
Thanks for the review. I'd missed that case and I agree bad things would
happen if state.hasOutboundRemaining() happened to be true while a read
was completing.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch 9.0.x updated: Additional fixes for 66076
Posted by Rémy Maucherat <re...@apache.org>.
On Tue, May 24, 2022 at 6:46 PM <ma...@apache.org> wrote:
>
> This is an automated email from the ASF dual-hosted git repository.
>
> markt pushed a commit to branch 9.0.x
> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>
>
> The following commit(s) were added to refs/heads/9.0.x by this push:
> new 680db4448d Additional fixes for 66076
> 680db4448d is described below
>
> commit 680db4448d392752fb57b91639e7ab34a3f58105
> Author: Mark Thomas <ma...@apache.org>
> AuthorDate: Tue May 24 17:46:20 2022 +0100
>
> Additional fixes for 66076
>
> The vectored IO version of the fix.
> ---
> java/org/apache/tomcat/util/net/AprEndpoint.java | 7 +++++++
> java/org/apache/tomcat/util/net/Nio2Endpoint.java | 7 +++++++
> java/org/apache/tomcat/util/net/NioEndpoint.java | 19 ++++++++++++-------
> .../org/apache/tomcat/util/net/SocketWrapperBase.java | 4 +++-
> 4 files changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/java/org/apache/tomcat/util/net/AprEndpoint.java b/java/org/apache/tomcat/util/net/AprEndpoint.java
> index c02e90fc09..20d10efa11 100644
> --- a/java/org/apache/tomcat/util/net/AprEndpoint.java
> +++ b/java/org/apache/tomcat/util/net/AprEndpoint.java
> @@ -2813,6 +2813,13 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
> return inline;
> }
>
> + @Override
> + protected boolean hasOutboundRemaining() {
> + // NIO2 never has remaining outbound data when the completion
> + // handler is called
> + return false;
> + }
> +
> @Override
> public void run() {
> // Perform the IO operation
> diff --git a/java/org/apache/tomcat/util/net/Nio2Endpoint.java b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
> index 49ee411016..c5b9a395f5 100644
> --- a/java/org/apache/tomcat/util/net/Nio2Endpoint.java
> +++ b/java/org/apache/tomcat/util/net/Nio2Endpoint.java
> @@ -1021,6 +1021,13 @@ public class Nio2Endpoint extends AbstractJsseEndpoint<Nio2Channel,AsynchronousS
> return Nio2Endpoint.isInline();
> }
>
> + @Override
> + protected boolean hasOutboundRemaining() {
> + // NIO2 never has remaining outbound data when the completion
> + // handler is called
> + return false;
> + }
> +
> @Override
> protected void start() {
> if (read) {
> diff --git a/java/org/apache/tomcat/util/net/NioEndpoint.java b/java/org/apache/tomcat/util/net/NioEndpoint.java
> index b8b6d4339d..ea65dd6600 100644
> --- a/java/org/apache/tomcat/util/net/NioEndpoint.java
> +++ b/java/org/apache/tomcat/util/net/NioEndpoint.java
> @@ -1622,6 +1622,11 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel>
> return inline;
> }
>
> + @Override
> + protected boolean hasOutboundRemaining() {
> + return getSocket().getOutboundRemaining() > 0;
> + }
> +
> @Override
> public void run() {
> // Perform the IO operation
> @@ -1654,13 +1659,13 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel>
> } else {
> boolean doWrite = true;
> // Write from main buffer first
> - if (!socketBufferHandler.isWriteBufferEmpty()) {
> + if (socketOrNetworkBufferHasDataLeft()) {
> // There is still data inside the main write buffer, it needs to be written first
> socketBufferHandler.configureWriteBufferForRead();
> do {
> nBytes = getSocket().write(socketBufferHandler.getWriteBuffer());
> - } while (!socketBufferHandler.isWriteBufferEmpty() && nBytes > 0);
> - if (!socketBufferHandler.isWriteBufferEmpty()) {
> + } while (socketOrNetworkBufferHasDataLeft() && nBytes > 0);
> + if (socketOrNetworkBufferHasDataLeft()) {
> doWrite = false;
> }
> // Preserve a negative value since it is an error
> @@ -1681,7 +1686,8 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel>
> updateLastWrite();
> }
> }
> - if (nBytes != 0 || !buffersArrayHasRemaining(buffers, offset, length)) {
> + if (nBytes != 0 || (!buffersArrayHasRemaining(buffers, offset, length) &&
> + !socketOrNetworkBufferHasDataLeft())) {
> completionDone = false;
> }
> }
> @@ -1689,7 +1695,8 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel>
> setError(e);
> }
> }
> - if (nBytes > 0 || (nBytes == 0 && !buffersArrayHasRemaining(buffers, offset, length))) {
> + if (nBytes > 0 || (nBytes == 0 && !buffersArrayHasRemaining(buffers, offset, length) &&
> + !socketOrNetworkBufferHasDataLeft())) {
> // The bytes processed are only updated in the completion handler
> completion.completed(Long.valueOf(nBytes), this);
> } else if (nBytes < 0 || getError() != null) {
> @@ -1708,9 +1715,7 @@ public class NioEndpoint extends AbstractJsseEndpoint<NioChannel,SocketChannel>
> }
> }
> }
> -
> }
> -
> }
>
>
> diff --git a/java/org/apache/tomcat/util/net/SocketWrapperBase.java b/java/org/apache/tomcat/util/net/SocketWrapperBase.java
> index bcb0119fc6..d95a31a15d 100644
> --- a/java/org/apache/tomcat/util/net/SocketWrapperBase.java
> +++ b/java/org/apache/tomcat/util/net/SocketWrapperBase.java
> @@ -1036,6 +1036,8 @@ public abstract class SocketWrapperBase<E> {
> */
> protected abstract boolean isInline();
>
> + protected abstract boolean hasOutboundRemaining();
> +
> /**
> * Process the operation using the connector executor.
> * @return true if the operation was accepted, false if the executor
> @@ -1087,7 +1089,7 @@ public abstract class SocketWrapperBase<E> {
> boolean completion = true;
> if (state.check != null) {
> CompletionHandlerCall call = state.check.callHandler(currentState, state.buffers, state.offset, state.length);
> - if (call == CompletionHandlerCall.CONTINUE) {
> + if (call == CompletionHandlerCall.CONTINUE || state.hasOutboundRemaining()) {
I understand why it's there (the operation will indeed seem complete
in that case, but it's not fully complete), but it needs to check
state.read then (looping a read operation will most likely not do
anything good; maybe it shouldn't happen, but it's a lot better to be
safe), so:
if (call == CompletionHandlerCall.CONTINUE || (!state.read &&
state.hasOutboundRemaining())) {
Overall the change is more convoluted than I expected, and I would
have messed it up without a test case.
Rémy
> complete = false;
> } else if (call == CompletionHandlerCall.NONE) {
> completion = false;
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch 9.0.x updated: Additional fixes for 66076
Posted by Mark Thomas <ma...@apache.org>.
On 24/05/2022 18:35, Christopher Schultz wrote:
<snip/>
>> diff --git a/java/org/apache/tomcat/util/net/AprEndpoint.java
>> b/java/org/apache/tomcat/util/net/AprEndpoint.java
>> index c02e90fc09..20d10efa11 100644
>> --- a/java/org/apache/tomcat/util/net/AprEndpoint.java
>> +++ b/java/org/apache/tomcat/util/net/AprEndpoint.java
>> @@ -2813,6 +2813,13 @@ public class AprEndpoint extends
>> AbstractEndpoint<Long,Long> implements SNICallB
>> return inline;
>> }
>> + @Override
>> + protected boolean hasOutboundRemaining() {
>> + // NIO2 never has remaining outbound data when the
>> completion
>> + // handler is called
>> + return false;
>> + }
>
> s/NIO2/APR/?
Yes.
> Should this just be the default implementation instead of an abstract
> superclass method and two separate -nothing implementations?
I think so. It is less code which is usually how I think about this sort
of thing. I'll fix that before any back-port (assuming the fix is good).
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch 9.0.x updated: Additional fixes for 66076
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 5/24/22 12:46, markt@apache.org wrote:
> This is an automated email from the ASF dual-hosted git repository.
>
> markt pushed a commit to branch 9.0.x
> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>
>
> The following commit(s) were added to refs/heads/9.0.x by this push:
> new 680db4448d Additional fixes for 66076
> 680db4448d is described below
>
> commit 680db4448d392752fb57b91639e7ab34a3f58105
> Author: Mark Thomas <ma...@apache.org>
> AuthorDate: Tue May 24 17:46:20 2022 +0100
>
> Additional fixes for 66076
>
> The vectored IO version of the fix.
> ---
> java/org/apache/tomcat/util/net/AprEndpoint.java | 7 +++++++
> java/org/apache/tomcat/util/net/Nio2Endpoint.java | 7 +++++++
> java/org/apache/tomcat/util/net/NioEndpoint.java | 19 ++++++++++++-------
> .../org/apache/tomcat/util/net/SocketWrapperBase.java | 4 +++-
> 4 files changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/java/org/apache/tomcat/util/net/AprEndpoint.java b/java/org/apache/tomcat/util/net/AprEndpoint.java
> index c02e90fc09..20d10efa11 100644
> --- a/java/org/apache/tomcat/util/net/AprEndpoint.java
> +++ b/java/org/apache/tomcat/util/net/AprEndpoint.java
> @@ -2813,6 +2813,13 @@ public class AprEndpoint extends AbstractEndpoint<Long,Long> implements SNICallB
> return inline;
> }
>
> + @Override
> + protected boolean hasOutboundRemaining() {
> + // NIO2 never has remaining outbound data when the completion
> + // handler is called
> + return false;
> + }
s/NIO2/APR/?
Should this just be the default implementation instead of an abstract
superclass method and two separate -nothing implementations?
-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org