Re: [PR] Fix NioChannel's toString() throwing NullPointerException in some cases [tomcat]

2023-10-25 Thread via GitHub


chenggwang commented on PR #671:
URL: https://github.com/apache/tomcat/pull/671#issuecomment-1780424297

   > Thanks for the PR.
   Thanks for all the REVIEWS!
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


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



Re: [PR] Fix NioChannel's toString() throwing NullPointerException in some cases [tomcat]

2023-10-25 Thread via GitHub


aooohan commented on PR #671:
URL: https://github.com/apache/tomcat/pull/671#issuecomment-1780324734

   Thanks for the PR.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


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



Re: [PR] Fix NioChannel's toString() throwing NullPointerException in some cases [tomcat]

2023-10-25 Thread via GitHub


aooohan merged PR #671:
URL: https://github.com/apache/tomcat/pull/671


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


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



Re: [PR] Fix NioChannel's toString() throwing NullPointerException in some cases [tomcat]

2023-10-14 Thread via GitHub


chenggwang commented on PR #671:
URL: https://github.com/apache/tomcat/pull/671#issuecomment-1763278643

   > If we are going to do anything for this
   > 
   > > Just simple `... + ":" + sc;` is enough
   > 
   > works for me if applied to both NIO and NIO2 rather than just NIO
   
   Ok!Mark,I've modified NIO and NIO2 based on this suggestion, and the test 
for NIO2 passes (it's simpler I won't write a test case, I'll just post a 
picture)
   https://github.com/apache/tomcat/assets/90715678/2cbbb3b8-2aff-492b-ade2-9ceffc949d9a;>
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


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



Re: [PR] Fix NioChannel's toString() throwing NullPointerException in some cases [tomcat]

2023-10-14 Thread via GitHub


markt-asf commented on PR #671:
URL: https://github.com/apache/tomcat/pull/671#issuecomment-1763192873

   If we are going to do anything for this
   
   > Just simple `... + ":" + sc;` is enough
   
   works for me if applied to both NIO and NIO2 rather than just NIO.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


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



Re: [PR] Fix NioChannel's toString() throwing NullPointerException in some cases [tomcat]

2023-10-14 Thread via GitHub


chenggwang commented on PR #671:
URL: https://github.com/apache/tomcat/pull/671#issuecomment-1763157260

   This is a bug that tomcat itself just doesn't realize. Because it doesn't 
need to call toString immediately after the `NioChannel` object is 
created.However, for third-party software that wants to monitor the operation 
of the object, it will start logging information as soon as the object is 
created. The external system doesn't know that this is a “half-finished object” 
and must wait until the rest method is executed before calling toString.
   
   This fix may not help much with tomcat itself, but that shouldn't stop we 
from fixing it. After all, it couldn't be a better fix for an external 
monitoring system. Besides, fixing it won't do any harm to tomcat itself.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


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



Re: [PR] Fix NioChannel's toString() throwing NullPointerException in some cases [tomcat]

2023-10-14 Thread via GitHub


chenggwang commented on PR #671:
URL: https://github.com/apache/tomcat/pull/671#issuecomment-1763120686

   > I do not like the use of "?:" operator here. Just simple `... + ":" + sc;` 
is enough, or if you want to be explicit `+ ":" + String.valueOf(sc);`
   
   If we don't consider the constant pool memory consumption, and readability 
(String + Object, which doesn't clearly express the intent). Your method is 
fine this way. In fact this will end up with one more entry in the constant 
pool. like this
   `#1 = Methodref  #xx.#xxx // 
java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;`
   `#2 = Methodref  #xx.#xxx  // 
java/lang/StringBuilder.append:(Ljava/lang/Object;)Ljava/lang/StringBuilder;`
   
   `super.toString() + sc`, the first one is a string, the second one is an 
object,So two entries are needed.If they are all strings, only entry #1 is 
needed.Strong transfer object type, not recommended for + reasons . This is 
just a lazy approach.Avoid this if there are a lot of character operations in a 
lot of classes (message middleware, logging systems, etc.), memory is also 
precious!
   As for String.valueOf(sc), sc can't be null, direct NPE, not to mention it's 
a series of method stack calls, less efficient.
   Thank you for your input. The main discussion is about the need to ensure 
that toString() is available to the public immediately after the object is 
created, and I'm encountering NPEs caused by it in my project that monitors 
object lifecycles and buries it, do you have any good ideas?@kkolinko
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


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



Re: [PR] Fix NioChannel's toString() throwing NullPointerException in some cases [tomcat]

2023-10-14 Thread via GitHub


kkolinko commented on PR #671:
URL: https://github.com/apache/tomcat/pull/671#issuecomment-1763032642

   I do not like the use of "?:" operator here.
   Just simple `... + ":" + sc;`  is enough, or if you want to be explicit `+ 
":" + String.valueOf(sc);`


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


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



Re: [PR] Fix NioChannel's toString() throwing NullPointerException in some cases [tomcat]

2023-10-14 Thread via GitHub


chenggwang commented on PR #671:
URL: https://github.com/apache/tomcat/pull/671#issuecomment-1762757733

   @rmaucher @markt-asf @michael-o Such a change in tomcat functionality does 
not help, but after the initialization of the NioChannel object should provide 
a reliable, stable function, can not allow the caller to trigger an error 
resulting in the termination of the program, the current large and complex 
network, the monitoring of the infrastructure is particularly important at this 
time, the role of the toString is very obvious, if you modify the words, for 
the NIO2channel of the toString need to be the same change.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


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



Re: [PR] Fix NioChannel's toString() throwing NullPointerException in some cases [tomcat]

2023-10-14 Thread via GitHub


chenggwang commented on PR #671:
URL: https://github.com/apache/tomcat/pull/671#issuecomment-1762749332

   > The benefit seems null IMO, in the debugger it will print out that this is 
null instead of a NPE, which should mean the same for a developer. Also, 
Nio2Channel.toString is the same.
   
   Following is the`NioChannel` attribute `SocketChannel sc`, along with its 
`constructor`, `rest` methods and `toString` method.We initialize a 
`NioChannel` through the constructor, only the `bufHandler` property is 
assigned a value, and its `sc` property remains null. At this point the object 
is nearly created, meaning we can use its properties and methods as we wish. 
But when we use its toString() method, `super.toString() + ":" + 
sc.toString()`, `sc` is null, so it throws an NPE. The `sc` is not assigned a 
value until we call the reset method. Otherwise, sc.toString() throws NPE 
directly instead of printing null
   ```java
   public class NioChannel implements ByteChannel, ScatteringByteChannel, 
GatheringByteChannel {
   
   ... Omit extraneous code
   protected final SocketBufferHandler bufHandler;
   protected SocketChannel sc = null;
   protected NioSocketWrapper socketWrapper = null;
   public NioChannel(SocketBufferHandler bufHandler) {
   this.bufHandler = bufHandler;
   }
   ...
   public String toString() {
   return super.toString() + ":" + sc.toString();
  }
   public void reset(SocketChannel channel, NioSocketWrapper socketWrapper) 
throws IOException {
   this.sc = channel;
   this.socketWrapper = socketWrapper;
   bufHandler.reset();
   }
   ...Omit extraneous code
   
   }
   ```
   Going back to the object-oriented level, when we provide an object, we need 
to make sure that the basic functionality it provides to the outside world 
after initialization is complete is usable and reliable. The basic 
functionality of course includes the `toString()` method, because this is its 
calling card, through which the outside world should know the basic information 
about the object. This is also the original purpose of the toString method.
   
   On line 12 we create a channel with `createChannel(bufhandler)`,At this 
point we are calling the constructor of the NioChannel.This is the same 
explanation as I started with, sc is still null and all calls to channel's` 
toString` method before line 14 report NPE.
   Similarly `NioSocketWrapper newWrapper`, the `newWrapper.toString()` method 
throws an NPE until the rest method is called, as it looks like this 
`super.toString() + ":" + String.valueOf(socket)`, ` String.valueOf(socket)` 
still calls socket(NioChannel)'s `sc.toString`, when `sc` is null.
   ```java
   line 1 protected boolean setSocketOptions(SocketChannel socket) {
   line 2   NioSocketWrapper socketWrapper = null;
   line 3try {
   // Allocate channel and wrapper
   line 4NioChannel channel = null;
   line 5if (nioChannels != null) {
   line 6channel = nioChannels.pop();
   }
   line 7if (channel == null) {
   line 8SocketBufferHandler bufhandler = new 
SocketBufferHandler(
   line 9  socketProperties.getAppReadBufSize(),
   line 10socketProperties.getAppWriteBufSize(),
   line 11socketProperties.getDirectBuffer());
   line 12channel = createChannel(bufhandler);
   }
   line 13NioSocketWrapper newWrapper = new 
NioSocketWrapper(channel, this);
   line 14channel.reset(socket, newWrapper);
   line 15connections.put(socket, newWrapper);
   line 16socketWrapper = newWrapper;
   ```
   createChannel method
   ```java
   protected NioChannel createChannel(SocketBufferHandler buffer) {
   if (isSSLEnabled()) {
   return new SecureNioChannel(buffer, this);
   }
   return new NioChannel(buffer);
   }
   public NioChannel(SocketBufferHandler bufHandler) {
   this.bufHandler = bufHandler;
   }
   ```
   https://github.com/apache/tomcat/assets/90715678/44871f56-e5a2-435e-86cf-e15063806971;>
   
   Above is the `System.out.println` I added between line 13 and 14 to output 
the `channel` and `newWrapper` objects, throwing NPE's.I have a monitoring 
project that tracks changes in the life cycle of an object after any object is 
created, we are monitoring all constructors, reflections, and start logging as 
soon as an object is created. The two lines `System.out.println` in the diagram 
are similar to the abstraction of our monitoring function as a buried point. To 
clarify our buried points are at the bytecode level, there is no hardcoding of 
tomcat source code dumping. However, the NPE occurred when logging, so I went 
down to the source code to debug it, and found that it was the reason that 
toString() didn't judge the null.
   

Re: [PR] Fix NioChannel's toString() throwing NullPointerException in some cases [tomcat]

2023-10-13 Thread via GitHub


michael-o commented on PR #671:
URL: https://github.com/apache/tomcat/pull/671#issuecomment-1761094466

   > The benefit seems null IMO, in the debugger it will print out that this is 
null instead of a NPE, which should mean the same for a developer. Also, 
Nio2Channel.toString is the same.
   
   In terms of consistency we likely still should care, no?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


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



Re: [PR] Fix NioChannel's toString() throwing NullPointerException in some cases [tomcat]

2023-10-13 Thread via GitHub


rmaucher commented on PR #671:
URL: https://github.com/apache/tomcat/pull/671#issuecomment-1761084688

   The benefit seems null IMO, in the debugger it will print out that this is 
null instead of a NPE, which should mean the same for a developer. Also, 
Nio2Channel.toString is the same.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


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



Re: [PR] Fix NioChannel's toString() throwing NullPointerException in some cases [tomcat]

2023-10-13 Thread via GitHub


michael-o commented on PR #671:
URL: https://github.com/apache/tomcat/pull/671#issuecomment-1761073554

   @markt-asf Are we good to merge this one?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


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



Re: [PR] Fix NioChannel's toString() throwing NullPointerException in some cases [tomcat]

2023-10-11 Thread via GitHub


CharliesAngel1 commented on PR #671:
URL: https://github.com/apache/tomcat/pull/671#issuecomment-1756870192

   oook


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


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



Re: [PR] Fix NioChannel's toString() throwing NullPointerException in some cases [tomcat]

2023-10-10 Thread via GitHub


chenggwang commented on PR #671:
URL: https://github.com/apache/tomcat/pull/671#issuecomment-1756826381

   Can someone please review this?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


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



Re: [PR] Fix NioChannel's toString() throwing NullPointerException in some cases [tomcat]

2023-10-10 Thread via GitHub


chenggwang commented on PR #671:
URL: https://github.com/apache/tomcat/pull/671#issuecomment-1755896980

   ```java 
   protected boolean setSocketOptions(SocketChannel socket) {
   NioSocketWrapper socketWrapper = null;
   try {
   // Allocate channel and wrapper
   NioChannel channel = null;
   if (nioChannels != null) {
   channel = nioChannels.pop();
   }
   if (channel == null) {
   SocketBufferHandler bufhandler = new SocketBufferHandler(
   socketProperties.getAppReadBufSize(),
   socketProperties.getAppWriteBufSize(),
   socketProperties.getDirectBuffer());
   channel = createChannel(bufhandler);
   }
   NioSocketWrapper newWrapper = new NioSocketWrapper(channel, 
this);
   channel.reset(socket, newWrapper);
   connections.put(socket, newWrapper);
   socketWrapper = newWrapper;
   ```
   Only `channel.reset(socket, newWrapper)` is executed. The sc, newWrapper 
attributes in channel are only assigned values.And chanel's toString method 
looks like this `super.toString() + ":" + sc.toString()` .sc.toString() then 
raises the null pointer during IDEA debugging.
   https://github.com/apache/tomcat/assets/90715678/40714216-66fb-41be-9c73-d839dca70119;>
   @markt-asf


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


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



Re: [PR] Fix NioChannel's toString() throwing NullPointerException in some cases [tomcat]

2023-10-10 Thread via GitHub


chenggwang commented on PR #671:
URL: https://github.com/apache/tomcat/pull/671#issuecomment-1755822815

   `protected boolean setSocketOptions(SocketChannel socket) {
   NioSocketWrapper socketWrapper = null;
   try {
   // Allocate channel and wrapper
   NioChannel channel = null;
   if (nioChannels != null) {
   channel = nioChannels.pop();
   }
   if (channel == null) {
   SocketBufferHandler bufhandler = new SocketBufferHandler(
   socketProperties.getAppReadBufSize(),
   socketProperties.getAppWriteBufSize(),
   socketProperties.getDirectBuffer());
   channel = createChannel(bufhandler);
   }
   NioSocketWrapper newWrapper = new NioSocketWrapper(channel, 
this);
   channel.reset(socket, newWrapper);
   connections.put(socket, newWrapper);
   socketWrapper = newWrapper;`
   Because it is not until `channel.reset(socket, newWrapper); `is executed. sc 
and socketWrapper will not be assigned values. But if toString() is called 
before (e.g., channel, socketWrapper in IDEA breakpoint debugging), a null 
pointer exception is thrown.
   https://github.com/apache/tomcat/assets/90715678/a3d77ce8-74a2-46b1-aee7-f2a3cf6c2ade;>
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


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



[PR] Fix NioChannel's toString() throwing NullPointerException in some cases [tomcat]

2023-10-10 Thread via GitHub


chenggwang opened a new pull request, #671:
URL: https://github.com/apache/tomcat/pull/671

   NioChannel's toString() causes NioEndpoint's setSocketOptions method to 
throw a NullPointerException in some scenarios (e.g. idea breakpoint debugging).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


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