Re: RFR : 8024952 : ClassCastException in PlainSocketImpl.accept() when using custom socketImpl
Taken feedback on board. New webrev : http://cr.openjdk.java.net/~coffeys/webrev.8024952.2/webrev/ I've managed to get confirmation from original submitter that this works ok for them. regards, Sean. On 20/09/2013 11:29, Seán Coffey wrote: Dmitry, You're right. I was cautious in moving the code up but since we're pointing at FileDescriptor Objs, we should be ok. Daniel Fuchs has pointed out another issue. Null delegate being passed into impl.accept if dealing with a custom socketImpl! It should just be impl.accept(s); I'll get this corrected and tested. Thanks for pointers. Sean. On 20/09/13 10:20, Dmitry Samersoff wrote: Sean, It might be possible to set s.fd to delegate.fd before call to impl.accept and therefore merge if instanceOf block. -Dmitry On 2013-09-20 00:21, Seán Coffey wrote: Looking for review on recently reported issue. Issue seen on windows when a custom socketImpl is in use. bug report : https://bugs.openjdk.java.net/browse/JDK-8024952 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8024952/webrev/ Regards, Sean.
Re: RFR : 8024952 : ClassCastException in PlainSocketImpl.accept() when using custom socketImpl
On 01/10/2013 13:37, Daniel Fuchs wrote: Hi Seán, This looks simpler and better :-) However I wonder, do you still need to catch NPE in CustomSocketImplFactory.main ? Daniel, Since I'm only creating a dummy socketImpl to test the classcastexception, no real networking stack is in place here. I'm catching the NPE that would be thrown from the native Java_java_net_TwoStacksPlainSocketImpl_socketAccept function since the underlying socket passed to it is null. C:\tmpjava CustomSocketImplFactory Created Socket[addr=null,port=0,localport=0] Exception in thread main java.lang.NullPointerException: socket is null at java.net.TwoStacksPlainSocketImpl.socketAccept(Native Method) Regards, Sean. Or is that going to hide future bugs? best regards -- daniel (not a reviewer)
Re: RFR : 8024952 : ClassCastException in PlainSocketImpl.accept() when using custom socketImpl
On 10/1/13 3:09 PM, Seán Coffey wrote: Since I'm only creating a dummy socketImpl to test the classcastexception, no real networking stack is in place here. I'm catching the NPE that would be thrown from the native Java_java_net_TwoStacksPlainSocketImpl_socketAccept function since the underlying socket passed to it is null. C:\tmpjava CustomSocketImplFactory Created Socket[addr=null,port=0,localport=0] Exception in thread main java.lang.NullPointerException: socket is null at java.net.TwoStacksPlainSocketImpl.socketAccept(Native Method) That's what I would have expected from your previous changeset. But you're no longer passing null - right? Now you're passing an instance of CustomSocketImpl. So where does the NPE come from? Could it be because you should be calling: ServerSocket.setSocketImplFactory(new CustomSocketImplFactory()); and not: Socket.setSocketImplFactory(new CustomSocketImplFactory()); ? Or should you call both? best regards, -- daniel Regards, Sean. Or is that going to hide future bugs? best regards -- daniel (not a reviewer)
Re: RFR : 8024952 : ClassCastException in PlainSocketImpl.accept() when using custom socketImpl
On 10/1/13 1:53 PM, Seán Coffey wrote: Taken feedback on board. New webrev : http://cr.openjdk.java.net/~coffeys/webrev.8024952.2/webrev/ I've managed to get confirmation from original submitter that this works ok for them. regards, Sean. Hi Seán, This looks simpler and better :-) However I wonder, do you still need to catch NPE in CustomSocketImplFactory.main ? Or is that going to hide future bugs? best regards -- daniel (not a reviewer) On 20/09/2013 11:29, Seán Coffey wrote: Dmitry, You're right. I was cautious in moving the code up but since we're pointing at FileDescriptor Objs, we should be ok. Daniel Fuchs has pointed out another issue. Null delegate being passed into impl.accept if dealing with a custom socketImpl! It should just be impl.accept(s); I'll get this corrected and tested. Thanks for pointers. Sean. On 20/09/13 10:20, Dmitry Samersoff wrote: Sean, It might be possible to set s.fd to delegate.fd before call to impl.accept and therefore merge if instanceOf block. -Dmitry On 2013-09-20 00:21, Seán Coffey wrote: Looking for review on recently reported issue. Issue seen on windows when a custom socketImpl is in use. bug report : https://bugs.openjdk.java.net/browse/JDK-8024952 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8024952/webrev/ Regards, Sean.
Re: RFR : 8024952 : ClassCastException in PlainSocketImpl.accept() when using custom socketImpl
The changes look fine to me. -Chris. On 10/01/2013 12:53 PM, Seán Coffey wrote: Taken feedback on board. New webrev : http://cr.openjdk.java.net/~coffeys/webrev.8024952.2/webrev/ I've managed to get confirmation from original submitter that this works ok for them. regards, Sean. On 20/09/2013 11:29, Seán Coffey wrote: Dmitry, You're right. I was cautious in moving the code up but since we're pointing at FileDescriptor Objs, we should be ok. Daniel Fuchs has pointed out another issue. Null delegate being passed into impl.accept if dealing with a custom socketImpl! It should just be impl.accept(s); I'll get this corrected and tested. Thanks for pointers. Sean. On 20/09/13 10:20, Dmitry Samersoff wrote: Sean, It might be possible to set s.fd to delegate.fd before call to impl.accept and therefore merge if instanceOf block. -Dmitry On 2013-09-20 00:21, Seán Coffey wrote: Looking for review on recently reported issue. Issue seen on windows when a custom socketImpl is in use. bug report : https://bugs.openjdk.java.net/browse/JDK-8024952 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8024952/webrev/ Regards, Sean.
Re: RFR : 8024952 : ClassCastException in PlainSocketImpl.accept() when using custom socketImpl
Sean,
The fix looks good for me but the code might be better readable if you
inverse the condition.
if (!(s instanceof PlainSocketImpl)) {
impl.accept(s)
return;
}
... rest of the code
-Dmitry
On 2013-10-01 18:54, Daniel Fuchs wrote:
On 10/1/13 4:50 PM, Seán Coffey wrote:
On 01/10/2013 14:51, Daniel Fuchs wrote:
On 10/1/13 3:09 PM, Seán Coffey wrote:
Since I'm only creating a dummy socketImpl to test the
classcastexception, no real networking stack is in place here. I'm
catching the NPE that would be thrown from the native
Java_java_net_TwoStacksPlainSocketImpl_socketAccept function since the
underlying socket passed to it is null.
C:\tmpjava CustomSocketImplFactory
Created Socket[addr=null,port=0,localport=0]
Exception in thread main java.lang.NullPointerException: socket is
null
at java.net.TwoStacksPlainSocketImpl.socketAccept(Native
Method)
That's what I would have expected from your previous changeset.
But you're no longer passing null - right? Now you're passing
an instance of CustomSocketImpl.
So where does the NPE come from? Could it be because you should
be calling:
ServerSocket.setSocketImplFactory(new CustomSocketImplFactory());
and not:
Socket.setSocketImplFactory(new CustomSocketImplFactory()); ?
The original bug stemmed from a custom socket Impl interacting with the
JDK ServerSocket Impl. If I move both Socket and ServerSocket factory
implementations over to use the custom Impl, the testcase doesn't get to
walk through the JDK serverSocket code and the ClassCastException is not
seen.
The NPE seen is coming from down in the native stack when the JDK
ServerSocket is running through accept request (from our client socket).
I don't think it's an issue for this case.
line 611 :
http://hg.openjdk.java.net/jdk8/tl/jdk/file/tip/src/windows/native/java/net/TwoStacksPlainSocketImpl.c
regards,
Sean.
Thanks Seán. You convinced me.
-- daniel
Or should you call both?
best regards,
-- daniel
Regards,
Sean.
Or is that going to hide future bugs?
best regards
-- daniel (not a reviewer)
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Re: RFR : 8024952 : ClassCastException in PlainSocketImpl.accept() when using custom socketImpl
Sean, It might be possible to set s.fd to delegate.fd before call to impl.accept and therefore merge if instanceOf block. -Dmitry On 2013-09-20 00:21, Seán Coffey wrote: Looking for review on recently reported issue. Issue seen on windows when a custom socketImpl is in use. bug report : https://bugs.openjdk.java.net/browse/JDK-8024952 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8024952/webrev/ Regards, Sean. -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the source code.
Re: RFR : 8024952 : ClassCastException in PlainSocketImpl.accept() when using custom socketImpl
Dmitry, You're right. I was cautious in moving the code up but since we're pointing at FileDescriptor Objs, we should be ok. Daniel Fuchs has pointed out another issue. Null delegate being passed into impl.accept if dealing with a custom socketImpl! It should just be impl.accept(s); I'll get this corrected and tested. Thanks for pointers. Sean. On 20/09/13 10:20, Dmitry Samersoff wrote: Sean, It might be possible to set s.fd to delegate.fd before call to impl.accept and therefore merge if instanceOf block. -Dmitry On 2013-09-20 00:21, Seán Coffey wrote: Looking for review on recently reported issue. Issue seen on windows when a custom socketImpl is in use. bug report : https://bugs.openjdk.java.net/browse/JDK-8024952 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8024952/webrev/ Regards, Sean.
