Re: RFR [11] 8198358 : Align organization of DualStackPlainSocketImpl with TwoStacksPlainSocketImp [win]

[Alan Bateman]

On 25/03/2018 19:13, Ivan Gerasimov wrote:

:

In the code above, newfd was obtained from a call to accept() a few 
lines before this check.
So, the Java code has no way of being aware of this socket, and it 
will never be closed unless we do it right here, before returning -1.



The SocketImpl's fd is set with this code:
  (*env)->SetIntField(env, socketFdObj, IO_fd_fdID, fd);

If there is any possibility of returning without a pending exception it 
will be registered with the cleaner.


At this point, I think we have to treat all code calling NET_SocketClose 
as a suspect until the regression is tracked down.


-Alan

Re: RFR [11] 8198358 : Align organization of DualStackPlainSocketImpl with TwoStacksPlainSocketImp [win]

[Ivan Gerasimov]

Hi Chris!


On 3/25/18 6:37 AM, Chris Hegarty wrote:


On 23 Mar 2018, at 21:55, Chris Hegarty > wrote:



For reference, here's the combined webrev:
http://cr.openjdk.java.net/~igerasim/8198358/03/webrev/ 



I think this is good. And thanks for the additional test coverage, 
this is great.


Actually, there is a couple of places in the new native code that
has the following pattern:

 if (sa.sa.sa_family != AF_INET) {
 JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException",
 "Protocol family not supported");
 NET_SocketClose(newfd);
 return -1;
   }

I would like to remove the NET_SocketClose, and allow the
exception propagating at the Java-level be responsible for
closing the socket, which will then do so in cooperation
with the socket cleaner ( it will effectively cancel the cleaner ).

-Chris.

In the code above, newfd was obtained from a call to accept() a few 
lines before this check.
So, the Java code has no way of being aware of this socket, and it will 
never be closed unless we do it right here, before returning -1.


I checked the other places, where family is checked to be AF_INET and 
can confirm there's no other places where a socket gets closed.


With kind regards,
Ivan


--
With kind regards,
Ivan Gerasimov

Re: RFR [11] 8198358 : Align organization of DualStackPlainSocketImpl with TwoStacksPlainSocketImp [win]

[Chris Hegarty]

> On 23 Mar 2018, at 21:55, Chris Hegarty  wrote:
> 
>> For reference, here's the combined webrev:
>> http://cr.openjdk.java.net/~igerasim/8198358/03/webrev/ 
>> 
> 
> I think this is good. And thanks for the additional test coverage, this is 
> great.

Actually, there is a couple of places in the new native code that
has the following pattern:

 if (sa.sa.sa_family != AF_INET) {
 JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException",
 "Protocol family not supported");
 NET_SocketClose(newfd);
 return -1;
   }

I would like to remove the NET_SocketClose, and allow the
exception propagating at the Java-level be responsible for 
closing the socket, which will then do so in cooperation
with the socket cleaner ( it will effectively cancel the cleaner ).

-Chris.