Re: RFR 8216978: Drop support for pre JDK 1.4 SocketImpl implementations
On 29/04/2019 17:47, Michael McMahon wrote: : It still ends up as a close of the socket's file descriptor at the OS level one way or the other. Closing a socket's InputStream or OutputStream never resulted in a shutdown() call to the OS. If you want socket shutdown then you need to call shutdownInput() or shutdownOutput API on j.n.Socket directly. Right, I suspect David may have mis-read the discussion as it may not be obvious that Socket::getInputStream and Socket::getOutputStream return streams that implement close to close the Socket. One detail on shutdown is that the close may shutdown the stream for writing as part of the close. This is normal and long standing behavior for cases where SO_LINGER has not been set. -Alan
Re: RFR 8216978: Drop support for pre JDK 1.4 SocketImpl implementations
On 29/04/2019, 12:17, Alan Bateman wrote: On 29/04/2019 10:52, Michael McMahon wrote: Hi, This is another change which is part of the general cleanup of SocketImpls. The change removes support for pre JDK 1.4 socketimpls which do not implement the timed connect method. These SocketImpls have not been compilable since 1.4 and limited runtime support has been provided since then, which is now being removed. Webrev --- http://cr.openjdk.java.net/~michaelm/8216978/webrev.1/ This is a good cleanup. Changing SIS.close and SOS.close to caller super.close raises a number of questions. These close should never be called Socket.getInputStream and getOutputStream don't leak these streams to user code (they used to but now in JDK 13). My concern is that if they were ever to be called then it will be calling the FIS/FOS close methods which brings along a several questions on it interacts with the cleaner mechanism used by those classes. I don't think AbstractPlainSocketImpl.isBound needs to be volatile as it is guarded by the synchronization on the impl (the doConnect and bind methods are synchronized). ok The Windows version of PlainSocketImpl shouldn't need the constructor to be public (I see the Unix version is package-private). ok Should we use the opportunity to add something to the javadoc for the SocketImpl constructor so it's not empty? It can be as simple as "Initialize a new instance of this class" as used in other places where there isn't anything to say. The empty comment block looks odd I admit. It was needed to make it build and I left it empty so that the generated apidoc would not change. However, I guess I can add the above suggestion to the CSR Thanks, Michael
Re: RFR 8216978: Drop support for pre JDK 1.4 SocketImpl implementations
On 29/04/2019, 17:35, David Lloyd wrote: On Mon, Apr 29, 2019 at 10:54 AM Michael McMahon wrote: On 29/04/2019, 13:08, Chris Hegarty wrote: On 29/04/2019 12:17, Alan Bateman wrote: ... Changing SIS.close and SOS.close to caller super.close raises a number of questions. These close should never be called Socket.getInputStream and getOutputStream don't leak these streams to user code (they used to but now in JDK 13). My concern is that if they were ever to be called then it will be calling the FIS/FOS close methods which brings along a several questions on it interacts with the cleaner mechanism used by those classes. Since 8220493, these socket in/out stream close methods are effectively no longer in charge of closing the socket ( since that will happen from the outer stream wrapper in Socket ). I wonder if they should simply call impl.close()? On that point. Since it might not be obvious that SIS.close and SOS.close are no longer used, it might be more useful to just replace the close implementations with a comment and an assertion which underlines that fact. As someone who does a lot of socket programming (albeit mostly non-blocking), I would be *shocked* if closing a socket input or output stream didn't translate into shutting down input or output (respectively). Not close per se but socket shutdown. It still ends up as a close of the socket's file descriptor at the OS level one way or the other. Closing a socket's InputStream or OutputStream never resulted in a shutdown() call to the OS. If you want socket shutdown then you need to call shutdownInput() or shutdownOutput API on j.n.Socket directly. - Michael.
Re: RFR 8216978: Drop support for pre JDK 1.4 SocketImpl implementations
On Mon, Apr 29, 2019 at 10:54 AM Michael McMahon wrote: > > > > On 29/04/2019, 13:08, Chris Hegarty wrote: > > > > On 29/04/2019 12:17, Alan Bateman wrote: > >> ... > >> Changing SIS.close and SOS.close to caller super.close raises a > >> number of questions. These close should never be called > >> Socket.getInputStream and getOutputStream don't leak these streams to > >> user code (they used to but now in JDK 13). My concern is that if > >> they were ever to be called then it will be calling the FIS/FOS close > >> methods which brings along a several questions on it interacts with > >> the cleaner mechanism used by those classes. > > > > Since 8220493, these socket in/out stream close methods are effectively > > no longer in charge of closing the socket ( since that will happen from > > the outer stream wrapper in Socket ). I wonder if they should simply > > call impl.close()? > > > > On that point. Since it might not be obvious that SIS.close and > SOS.close are no longer used, > it might be more useful to just replace the close implementations with a > comment and an assertion > which underlines that fact. As someone who does a lot of socket programming (albeit mostly non-blocking), I would be *shocked* if closing a socket input or output stream didn't translate into shutting down input or output (respectively). Not close per se but socket shutdown. -- - DML
Re: RFR 8216978: Drop support for pre JDK 1.4 SocketImpl implementations
On 29/04/2019, 13:08, Chris Hegarty wrote: On 29/04/2019 12:17, Alan Bateman wrote: ... Changing SIS.close and SOS.close to caller super.close raises a number of questions. These close should never be called Socket.getInputStream and getOutputStream don't leak these streams to user code (they used to but now in JDK 13). My concern is that if they were ever to be called then it will be calling the FIS/FOS close methods which brings along a several questions on it interacts with the cleaner mechanism used by those classes. Since 8220493, these socket in/out stream close methods are effectively no longer in charge of closing the socket ( since that will happen from the outer stream wrapper in Socket ). I wonder if they should simply call impl.close()? On that point. Since it might not be obvious that SIS.close and SOS.close are no longer used, it might be more useful to just replace the close implementations with a comment and an assertion which underlines that fact. - Michael
Re: RFR 8216978: Drop support for pre JDK 1.4 SocketImpl implementations
On 29/04/2019 13:24, Alan Bateman wrote: 433 protected synchronized void bind(InetAddress address, int lport) [...] which for me justifies that it should be volatile. I think you are might be mixing up the lock on the vs. fdLock. From what I can tell, isBound is only accessed by doConnect and bind and they are both synchronized methods Darn. My mistake. I see it now :-( -- daniel
Re: RFR 8216978: Drop support for pre JDK 1.4 SocketImpl implementations
On 29/04/2019 13:00, Daniel Fuchs wrote:
Hi Alan,
On 29/04/2019 12:17, Alan Bateman wrote:
I don't think AbstractPlainSocketImpl.isBound needs to be volatile as
it is guarded by the synchronization on the impl (the doConnect and
bind methods are synchronized).
I see that it is set outside of any synchronized block
in AbstractPlainSocketImpl::bind
433 protected synchronized void bind(InetAddress address, int lport)
434 throws IOException
435 {
436 synchronized (fdLock) {
437 if (!closePending && !isBound) {
438 NetHooks.beforeTcpBind(fd, address, lport);
439 }
440 }
441 socketBind(address, lport);
442 isBound = true;
443 }
which for me justifies that it should be volatile.
I think you are might be mixing up the lock on the vs. fdLock. From what
I can tell, isBound is only accessed by doConnect and bind and they are
both synchronized methods.
-Alan
Re: RFR 8216978: Drop support for pre JDK 1.4 SocketImpl implementations
On 29/04/2019 12:17, Alan Bateman wrote: ... Changing SIS.close and SOS.close to caller super.close raises a number of questions. These close should never be called Socket.getInputStream and getOutputStream don't leak these streams to user code (they used to but now in JDK 13). My concern is that if they were ever to be called then it will be calling the FIS/FOS close methods which brings along a several questions on it interacts with the cleaner mechanism used by those classes. Since 8220493, these socket in/out stream close methods are effectively no longer in charge of closing the socket ( since that will happen from the outer stream wrapper in Socket ). I wonder if they should simply call impl.close()? -Chris.
Re: RFR 8216978: Drop support for pre JDK 1.4 SocketImpl implementations
Hi Alan,
On 29/04/2019 12:17, Alan Bateman wrote:
I don't think AbstractPlainSocketImpl.isBound needs to be volatile as it
is guarded by the synchronization on the impl (the doConnect and bind
methods are synchronized).
I see that it is set outside of any synchronized block
in AbstractPlainSocketImpl::bind
433 protected synchronized void bind(InetAddress address, int lport)
434 throws IOException
435 {
436synchronized (fdLock) {
437 if (!closePending && !isBound) {
438 NetHooks.beforeTcpBind(fd, address, lport);
439 }
440 }
441 socketBind(address, lport);
442 isBound = true;
443 }
which for me justifies that it should be volatile.
best regards,
-- daniel
Re: RFR 8216978: Drop support for pre JDK 1.4 SocketImpl implementations
On 29/04/2019 10:52, Michael McMahon wrote: Hi, This is another change which is part of the general cleanup of SocketImpls. The change removes support for pre JDK 1.4 socketimpls which do not implement the timed connect method. These SocketImpls have not been compilable since 1.4 and limited runtime support has been provided since then, which is now being removed. Webrev --- http://cr.openjdk.java.net/~michaelm/8216978/webrev.1/ This is a good cleanup. Changing SIS.close and SOS.close to caller super.close raises a number of questions. These close should never be called Socket.getInputStream and getOutputStream don't leak these streams to user code (they used to but now in JDK 13). My concern is that if they were ever to be called then it will be calling the FIS/FOS close methods which brings along a several questions on it interacts with the cleaner mechanism used by those classes. I don't think AbstractPlainSocketImpl.isBound needs to be volatile as it is guarded by the synchronization on the impl (the doConnect and bind methods are synchronized). The Windows version of PlainSocketImpl shouldn't need the constructor to be public (I see the Unix version is package-private). Should we use the opportunity to add something to the javadoc for the SocketImpl constructor so it's not empty? It can be as simple as "Initialize a new instance of this class" as used in other places where there isn't anything to say. -Alan.
Re: RFR 8216978: Drop support for pre JDK 1.4 SocketImpl implementations
Hi Daniel, On 29/04/2019, 11:34, Daniel Fuchs wrote: Hi Michael, I'm too new to networking libs to actually review this change. However I have eyeballed it and it looks like a very nice simplification and cleanup! I didn't see anything that looked wrong. Two thing though: java/net/Socket.java: (and at multiple other places in this file) 1615 // Before 1.3 Sockets were always connected during creation I believe that with your change this comment is now obsolete. Maybe it should be altered / removed? AbsctractSocketImpl/PlainSocketImpl: It's a bit surprising that createSocket has now acquired an isServer boolean, as it makes it look as if a value different than that given to the constructor could be passed in. Aren't the PlainSocketImpl subclasses all able to access this `isServer` field? I understand that the unix impl needs to pass the value of that field to the native, but maybe it could simply have a one arg java createSocket method that calls the underlying two args native impl? I think that's a fair point. Actually, the 'isServer' field was added after other changes had already been made. But, particularly since the field/parameter is not used on windows then it could be refactored such that it doesn't appear in Windows and the abstract socketCreate() method would not need it either. Thanks, Michael.
Re: RFR 8216978: Drop support for pre JDK 1.4 SocketImpl implementations
Thanks Chris. Comments noted. - Michael On 29/04/2019, 11:26, Chris Hegarty wrote: Michael, On 29/04/2019 10:52, Michael McMahon wrote: Hi, This is another change which is part of the general cleanup of SocketImpls. The change removes support for pre JDK 1.4 socketimpls which do not implement the timed connect method. These SocketImpls have not been compilable since 1.4 and limited runtime support has been provided since then, which is now being removed. Webrev --- http://cr.openjdk.java.net/~michaelm/8216978/webrev.1/ Mostly looks good. A few specific comments: Socket.java Please remove these comments, as they are no longer applicable: 1675 // Before 1.3 Sockets were always connected during creation 1692 // Before 1.3 Sockets were always bound during creation ServerSocket.java Please remove: 75 * Are we using an older SocketImpl? 76 */ 77 private boolean oldImpl = false; And there is still one reference to it: 347 if (!oldImpl && isBound()) SocketImpl.java Please remove: Defaults to false, unless setIsServer() called 79 * from ServerSocket -Chris.
Re: RFR 8216978: Drop support for pre JDK 1.4 SocketImpl implementations
Hi Michael, I'm too new to networking libs to actually review this change. However I have eyeballed it and it looks like a very nice simplification and cleanup! I didn't see anything that looked wrong. Two thing though: java/net/Socket.java: (and at multiple other places in this file) 1615 // Before 1.3 Sockets were always connected during creation I believe that with your change this comment is now obsolete. Maybe it should be altered / removed? AbsctractSocketImpl/PlainSocketImpl: It's a bit surprising that createSocket has now acquired an isServer boolean, as it makes it look as if a value different than that given to the constructor could be passed in. Aren't the PlainSocketImpl subclasses all able to access this `isServer` field? I understand that the unix impl needs to pass the value of that field to the native, but maybe it could simply have a one arg java createSocket method that calls the underlying two args native impl? best regards, -- daniel On 29/04/2019 10:52, Michael McMahon wrote: Hi, This is another change which is part of the general cleanup of SocketImpls. The change removes support for pre JDK 1.4 socketimpls which do not implement the timed connect method. These SocketImpls have not been compilable since 1.4 and limited runtime support has been provided since then, which is now being removed. Webrev --- http://cr.openjdk.java.net/~michaelm/8216978/webrev.1/ CSR https://bugs.openjdk.java.net/browse/JDK-8222546 Thanks, Michael.
Re: RFR 8216978: Drop support for pre JDK 1.4 SocketImpl implementations
Michael, On 29/04/2019 10:52, Michael McMahon wrote: Hi, This is another change which is part of the general cleanup of SocketImpls. The change removes support for pre JDK 1.4 socketimpls which do not implement the timed connect method. These SocketImpls have not been compilable since 1.4 and limited runtime support has been provided since then, which is now being removed. Webrev --- http://cr.openjdk.java.net/~michaelm/8216978/webrev.1/ Mostly looks good. A few specific comments: Socket.java Please remove these comments, as they are no longer applicable: 1675 // Before 1.3 Sockets were always connected during creation 1692 // Before 1.3 Sockets were always bound during creation ServerSocket.java Please remove: 75 * Are we using an older SocketImpl? 76 */ 77 private boolean oldImpl = false; And there is still one reference to it: 347 if (!oldImpl && isBound()) SocketImpl.java Please remove: Defaults to false, unless setIsServer() called 79 * from ServerSocket -Chris.
RFR 8216978: Drop support for pre JDK 1.4 SocketImpl implementations
Hi, This is another change which is part of the general cleanup of SocketImpls. The change removes support for pre JDK 1.4 socketimpls which do not implement the timed connect method. These SocketImpls have not been compilable since 1.4 and limited runtime support has been provided since then, which is now being removed. Webrev --- http://cr.openjdk.java.net/~michaelm/8216978/webrev.1/ CSR https://bugs.openjdk.java.net/browse/JDK-8222546 Thanks, Michael.
Re: RFR: 8129315: java/net/Socket/LingerTest.java and java/net/Socket/ShutdownBoth.java timeout intermittently
Thanks Alan! I have already pushed those changes - but I'll retain this trick for the next batch. FWIW I saw that the default value for the backlog in the impl was 50, so that's what I'd been using. But not having to specify it at all is indeed a better solution. best regards, -- daniel On 27/04/2019 15:29, Alan Bateman wrote: One minor comment is that the 3-arg constructor requires you to specify a value for the backlog (or <= 0 to use an implementation default). An alternative that I used in other tests is to use the no-arg constructor and then bind to the loopback. It just avoids need to think about the backlog, otherwise the same.
