Re: [tomcat] branch main updated: Clear SocketWrapper reference to help GC

[Christopher Schultz]

Han,

On 5/20/23 21:42, Han Li wrote:




On May 21, 2023, at 01:19, Mark Thomas  wrote:

On 19/05/2023 04:24, 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 

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 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.


This could be fixed by dropping the AJP connector in Tomcat 11.x :)

-chris

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: [tomcat] branch main updated: Clear SocketWrapper reference to help GC

[Han Li]

> On May 21, 2023, at 01:19, Mark Thomas  wrote:
> 
> On 19/05/2023 04:24, 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 
>> 
>> 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 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 
> 

Re: [tomcat] branch main updated: Clear SocketWrapper reference to help GC

[Rémy Maucherat]

On Sat, May 20, 2023 at 7:19 PM Mark Thomas  wrote:
>
> On 19/05/2023 04:24, 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
> >
> >
> > 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 
> > 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 {
> >  

Re: [tomcat] branch main updated: Clear SocketWrapper reference to help GC

[Mark Thomas]

On 19/05/2023 04:24, 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


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 
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.



  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
For additional commands, e-mail: dev-h...@tomcat.apache.org

[tomcat] branch main updated: Clear SocketWrapper reference to help GC

[lihan]

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 
AuthorDate: Fri May 19 11:24:27 2023 +0800

Clear SocketWrapper reference to help GC
---
 java/org/apache/coyote/AbstractProcessor.java  | 3 +++
 java/org/apache/coyote/http11/Http11Processor.java | 1 -
 java/org/apache/coyote/http2/StreamProcessor.java  | 6 +-
 java/org/apache/tomcat/util/net/Nio2Channel.java   | 1 +
 java/org/apache/tomcat/util/net/NioChannel.java| 1 +
 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