> On May 21, 2023, at 01:19, Mark Thomas <ma...@apache.org> wrote:
>
> On 19/05/2023 04:24, li...@apache.org <mailto:li...@apache.org> wrote:
>> This is an automated email from the ASF dual-hosted git repository.
>> lihan pushed a commit to branch main
>> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>> <https://gitbox.apache.org/repos/asf/tomcat.git>
>> The following commit(s) were added to refs/heads/main by this push:
>> new 10492dd22b Clear SocketWrapper reference to help GC
>> 10492dd22b is described below
>> commit 10492dd22bd64ff63cf77786fa67d45cdc2a54b3
>> Author: lihan <li...@apache.org <mailto:li...@apache.org>>
>> AuthorDate: Fri May 19 11:24:27 2023 +0800
>> Clear SocketWrapper reference to help GC
>
> -1. Veto.
>
>> ---
>> java/org/apache/coyote/AbstractProcessor.java | 3 +++
>> java/org/apache/coyote/http11/Http11Processor.java | 1 -
>> java/org/apache/coyote/http2/StreamProcessor.java | 6 +-----
>
> The above changes need to be reverted since they break AJP connections. The
> AJP processor is recycled after CPING MESSAGES and expects processing to
> continue afterwards. Setting the SocketWrapper to null triggers NPEs when
> trying to process the request that follows the CPING.
Sigh, I indeed forgot to test the AJP processor.
Have reverted, thanks for the review.
Han
>
>> java/org/apache/tomcat/util/net/Nio2Channel.java | 1 +
>> java/org/apache/tomcat/util/net/NioChannel.java | 1 +
>
> The above changes look to be OK although I'll note that they do add some
> processing overhead.
>
> Mark
>
>
>> 5 files changed, 6 insertions(+), 6 deletions(-)
>> diff --git a/java/org/apache/coyote/AbstractProcessor.java
>> b/java/org/apache/coyote/AbstractProcessor.java
>> index 3ee6898fbd..8965dab62b 100644
>> --- a/java/org/apache/coyote/AbstractProcessor.java
>> +++ b/java/org/apache/coyote/AbstractProcessor.java
>> @@ -721,6 +721,9 @@ public abstract class AbstractProcessor extends
>> AbstractProcessorLight implement
>> public void recycle() {
>> errorState = ErrorState.NONE;
>> asyncStateMachine.recycle();
>> + // Clear fields that can be cleared to aid GC and trigger NPEs if this
>> + // is reused
>> + socketWrapper = null;
>> }
>> diff --git a/java/org/apache/coyote/http11/Http11Processor.java
>> b/java/org/apache/coyote/http11/Http11Processor.java
>> index 9e2e74914c..85dfd34f3c 100644
>> --- a/java/org/apache/coyote/http11/Http11Processor.java
>> +++ b/java/org/apache/coyote/http11/Http11Processor.java
>> @@ -1418,7 +1418,6 @@ public class Http11Processor extends AbstractProcessor
>> {
>> inputBuffer.recycle();
>> outputBuffer.recycle();
>> upgradeToken = null;
>> - socketWrapper = null;
>> sendfileData = null;
>> sslSupport = null;
>> }
>> diff --git a/java/org/apache/coyote/http2/StreamProcessor.java
>> b/java/org/apache/coyote/http2/StreamProcessor.java
>> index 78cb770649..3b1c03ac38 100644
>> --- a/java/org/apache/coyote/http2/StreamProcessor.java
>> +++ b/java/org/apache/coyote/http2/StreamProcessor.java
>> @@ -392,8 +392,8 @@ class StreamProcessor extends AbstractProcessor {
>> @Override
>> public final void recycle() {
>> + super.recycle();
>> // StreamProcessor instances are not re-used.
>> -
>> // Calling removeRequestProcessor even though the RequestProcesser was
>> // never added will add the values from the RequestProcessor to the
>> // running total for the GlobalRequestProcessor
>> @@ -401,10 +401,6 @@ class StreamProcessor extends AbstractProcessor {
>> if (global != null) {
>> global.removeRequestProcessor(request.getRequestProcessor());
>> }
>> -
>> - // Clear fields that can be cleared to aid GC and trigger NPEs if this
>> - // is reused
>> - setSocketWrapper(null);
>> }
>> diff --git a/java/org/apache/tomcat/util/net/Nio2Channel.java
>> b/java/org/apache/tomcat/util/net/Nio2Channel.java
>> index 603c43f416..9d65266374 100644
>> --- a/java/org/apache/tomcat/util/net/Nio2Channel.java
>> +++ b/java/org/apache/tomcat/util/net/Nio2Channel.java
>> @@ -79,6 +79,7 @@ public class Nio2Channel implements
>> AsynchronousByteChannel {
>> @Override
>> public void close() throws IOException {
>> sc.close();
>> + reset(this.sc, null);
>> }
>> diff --git a/java/org/apache/tomcat/util/net/NioChannel.java
>> b/java/org/apache/tomcat/util/net/NioChannel.java
>> index d263ce9ae6..eae3f51171 100644
>> --- a/java/org/apache/tomcat/util/net/NioChannel.java
>> +++ b/java/org/apache/tomcat/util/net/NioChannel.java
>> @@ -81,6 +81,7 @@ public class NioChannel implements ByteChannel,
>> ScatteringByteChannel, Gathering
>> @Override
>> public void close() throws IOException {
>> sc.close();
>> + reset(this.sc,null);
>> }
>> /**
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
>> For additional commands, e-mail: dev-h...@tomcat.apache.org
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> <mailto:dev-unsubscr...@tomcat.apache.org>
> For additional commands, e-mail: dev-h...@tomcat.apache.org
> <mailto:dev-h...@tomcat.apache.org>
