Re: [PR] Fix NioChannel's toString() throwing NullPointerException in some cases [tomcat]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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
