On Sat, May 20, 2023 at 7:19 PM Mark Thomas <[email protected]> wrote:
>
> On 19/05/2023 04:24, [email protected] 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
> >
> >
> > 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 <[email protected]>
> > AuthorDate: Fri May 19 11:24:27 2023 +0800
> >
> > Clear SocketWrapper reference to help GC
>
> -1. Veto.
Seconded since the test failures caught this.
> > ---
> > 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.
>
> > 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.
I never did it on purpose since it seemed useless. However, I don't
think it really makes any difference.
Rémy
>
> 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: [email protected]
> > For additional commands, e-mail: [email protected]
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [email protected]
> For additional commands, e-mail: [email protected]
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
