Re: RFR 8153674: Expected SocketException not thrown when calling bind() with setReuseAddress(false)
Hi All, I had off line discussion here at Oracle and we decided not to override getReuseAddr/setReuseAddr for MulticastSocket. If user wants, he can set the SO_REUSEPORT with "setOption"before bind. For MulticastSocket SO_REUSEADDR&SO_REUSEPORT are semantically equivalent which means either option will be sufficient to indicate that the address&port is reusable. So setting SO_REUSEPORT in constructor is really necessary/required ? I am looking some comments on this from Intel people(they are in mail chain) who did this original change, before we decide how we wants to proceed on this issue. Thanks, Vyom On Wednesday 14 September 2016 08:47 PM, Chris Hegarty wrote: On 14/09/16 15:53, Mark Sheppard wrote: that's true wrt SO_REUSEPORT in MulticastSocket constructor. But the same could have been argued for the original invocation of setReuseAddress, by default , in the constructors, which is encapsulating, what pereceived as, common or defacto practice wrt applying SO_REUSEADDR on mcast sockets at the system level. As I understand it, it is generally perceived that SO_REUSEADDR and SO_REUSEPORT are semantically equivalent for multicast sockets. As such, I think in the case of MulticastSocket, the fact that the setRuseAddress() is called in the constructor, it is appropriate to set SO_REUSEPORT also when it exists in the OS. I take your point on the semantics of setReuseAddress in DatagramSocket as per its spec. The spec does allude to MulticastSocket. As such, the current proposal's changes just lack the appropriate javadoc to describe its behavior, and its additional functionality of setting SO_REUSEPORT. It is not necessary that overridden method should mirror the semantics of the base class method. If it is accepted that it is generally perceived that SO_REUSEADDR and SO_REUSEPORT are semantically equivalent for multicast sockets, then it seems appropriate that an overriding setReuseAddress(..) method in MulticastSocket can reflect this. That sounds reasonable. -Chris. regards Mark On 14/09/2016 14:58, Chris Hegarty wrote: One additional remark. Was it appropriate to update the legacy MC constructors to set the new JDK 9 SO_REUSEPORT in the first place? This can be achievable, opt-in from new code, by creating an unbound MS, setting the option, then binding. -Chris. On 14/09/16 14:47, Chris Hegarty wrote: Mark, On 14/09/16 14:22, Mark Sheppard wrote: Hi Chris, I don't fully understand your objections to the approach taken. Is there a compatibility issue with the addition of the additional methods to MulticastSocket? The concern is with setReuseAddress performing an operation that is not clear from the method specification, e.g. from setReuseAddress() * Enable/disable the SO_REUSEADDR socket option. This is no longer accurate. The proposed changes would affect SO_REUSEPORT too. I don't see Datagram.setReuseAddress(...) handling the SO_REUSEPORT option, this has to be done explicitly via setOption at this level of abstraction. Yes, it is a bit odd, but these are legacy classes. I am not opposed to adding a new method for this, or something else. I just want to avoid future issues and confusion when setReuseAddress is called and then it is noticed that, the somewhat orthogonal option, SO_REUSEPORT's value has changed. setReuseAddress's spec is very clear about what it does. MulticastSocket is a subclass of DatagramSocket (that in itself is a questionable structuring), and as such has specialized behaviour, and part of that specialization is the setting of the setting SO_REUSEADDR and SO_REUSEPORT in its constructors, to enable address reuse semantics, prior to binding an address. Understood. Of course, the setting of SO_REUSEPORT is new in 9, hence the problem. As part of that specialization, it would seem appropriate that MulticastSocket manipulate the SO_REUSEPORT option in a consistent way. Adding an overridden setReuseAddress(...) method provides that consistency and encapsulates the specialized behaviour. I agree with the abstraction, just not that setReuseAddress should be used to achieve it. The name and spec of this method is so tied to SO_REUSEADDR. Is alternatively proposal to NOT do anything to MulticastSocket, BUT document clearly how to handle the failing scenario, that an MulticastSocket requires both setReuseAddress() and a setOption call to disable reuseaddress semantics on an unbound MulticastSocket ? That is one option, and the option that I was suggesting as a possible alternative. This then raises the question of why have a convenience method, such as setReuseAddress() in the first place, when it can be handled adequately via the setOption We are moving away from these option specific getter and setter methods, in favor of the more general get/setOption methods, as the latter are more adaptable. If setReuseAddress is to operate on more than SO_REUSEADDR, then its spec should be very clear about this. -Chris.
Re: RFR 8153674: Expected SocketException not thrown when calling bind() with setReuseAddress(false)
Thanks Vyom, On 28/09/16 09:25, Vyom Tewari wrote: Hi All, I had off line discussion here at Oracle and we decided not to override getReuseAddr/setReuseAddr for MulticastSocket. If user wants, he can set the SO_REUSEPORT with "setOption"before bind. Right. All options that we looked at seem less than satisfactory ( these getter/setter methods work at a single socket option level ). For MulticastSocket SO_REUSEADDR&SO_REUSEPORT are semantically equivalent which means either option will be sufficient to indicate that the address&port is reusable. So setting SO_REUSEPORT in constructor is really necessary/required ? This is the crux of the issue. I have been unable to find the reasoning behind this particular part of the change for 6432031 [1]. As you correctly say, SO_REUSEADDR and SO_REUSEPORT are semantically equivalent for multicasting. I am looking some comments on this from Intel people(they are in mail chain) who did this original change, before we decide how we wants to proceed on this issue. At this point, without further clarification of why SO_REUSEPORT was added to the MulticastSocket constructor, my preference is to revert it. -Chris. [1] https://bugs.openjdk.java.net/browse/JDK-6432031 Thanks, Vyom On Wednesday 14 September 2016 08:47 PM, Chris Hegarty wrote: On 14/09/16 15:53, Mark Sheppard wrote: that's true wrt SO_REUSEPORT in MulticastSocket constructor. But the same could have been argued for the original invocation of setReuseAddress, by default , in the constructors, which is encapsulating, what pereceived as, common or defacto practice wrt applying SO_REUSEADDR on mcast sockets at the system level. As I understand it, it is generally perceived that SO_REUSEADDR and SO_REUSEPORT are semantically equivalent for multicast sockets. As such, I think in the case of MulticastSocket, the fact that the setRuseAddress() is called in the constructor, it is appropriate to set SO_REUSEPORT also when it exists in the OS. I take your point on the semantics of setReuseAddress in DatagramSocket as per its spec. The spec does allude to MulticastSocket. As such, the current proposal's changes just lack the appropriate javadoc to describe its behavior, and its additional functionality of setting SO_REUSEPORT. It is not necessary that overridden method should mirror the semantics of the base class method. If it is accepted that it is generally perceived that SO_REUSEADDR and SO_REUSEPORT are semantically equivalent for multicast sockets, then it seems appropriate that an overriding setReuseAddress(..) method in MulticastSocket can reflect this. That sounds reasonable. -Chris. regards Mark On 14/09/2016 14:58, Chris Hegarty wrote: One additional remark. Was it appropriate to update the legacy MC constructors to set the new JDK 9 SO_REUSEPORT in the first place? This can be achievable, opt-in from new code, by creating an unbound MS, setting the option, then binding. -Chris. On 14/09/16 14:47, Chris Hegarty wrote: Mark, On 14/09/16 14:22, Mark Sheppard wrote: Hi Chris, I don't fully understand your objections to the approach taken. Is there a compatibility issue with the addition of the additional methods to MulticastSocket? The concern is with setReuseAddress performing an operation that is not clear from the method specification, e.g. from setReuseAddress() * Enable/disable the SO_REUSEADDR socket option. This is no longer accurate. The proposed changes would affect SO_REUSEPORT too. I don't see Datagram.setReuseAddress(...) handling the SO_REUSEPORT option, this has to be done explicitly via setOption at this level of abstraction. Yes, it is a bit odd, but these are legacy classes. I am not opposed to adding a new method for this, or something else. I just want to avoid future issues and confusion when setReuseAddress is called and then it is noticed that, the somewhat orthogonal option, SO_REUSEPORT's value has changed. setReuseAddress's spec is very clear about what it does. MulticastSocket is a subclass of DatagramSocket (that in itself is a questionable structuring), and as such has specialized behaviour, and part of that specialization is the setting of the setting SO_REUSEADDR and SO_REUSEPORT in its constructors, to enable address reuse semantics, prior to binding an address. Understood. Of course, the setting of SO_REUSEPORT is new in 9, hence the problem. As part of that specialization, it would seem appropriate that MulticastSocket manipulate the SO_REUSEPORT option in a consistent way. Adding an overridden setReuseAddress(...) method provides that consistency and encapsulates the specialized behaviour. I agree with the abstraction, just not that setReuseAddress should be used to achieve it. The name and spec of this method is so tied to SO_REUSEADDR. Is alternatively proposal to NOT do anything to MulticastSocket, BUT document clearly how to handle the failing scenario, that an MulticastSocket requires both setReuseAddress()
RE: RFR(XS): 8166584: Remove obsolete utility function NET_ThrowSocketException in windows libnet
Thanks, I pushed: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/2b5229c75e93 Best regards Christoph > -Original Message- > From: Chris Hegarty [mailto:chris.hega...@oracle.com] > Sent: Dienstag, 27. September 2016 21:24 > To: Langer, Christoph > Cc: net-dev@openjdk.java.net > Subject: Re: RFR(XS): 8166584: Remove obsolete utility function > NET_ThrowSocketException in windows libnet > > > > On 27 Sep 2016, at 19:56, Langer, Christoph > wrote: > > > > Chris, > > > > thanks for your input. > > > > If there's no objections I'd push it like this later tomorrow: > > http://cr.openjdk.java.net/~clanger/webrevs/8166584.2/ > > Looks ok to me Christoph. > > Thanks, > -Chris. > > > I've replaced the JNU_JAVANETPKG and JNU_JAVAIOPKG macros with the full > exception class names. > > > > Best regards > > Christoph > > > >> -Original Message- > >> From: Chris Hegarty [mailto:chris.hega...@oracle.com] > >> Sent: Dienstag, 27. September 2016 10:10 > >> To: Langer, Christoph > >> Cc: net-dev@openjdk.java.net > >> Subject: Re: RFR(XS): 8166584: Remove obsolete utility function > >> NET_ThrowSocketException in windows libnet > >> > >> Christoph, > >> > >> On 26 Sep 2016, at 18:58, Langer, Christoph > >> wrote: > >>> > >>> Hi Chris, > >>> > >>> I agree with your comment on the NPE. It would probably be wrong. So I > >> restored the old code and also removed the comments suggesting the NPE. > >> Here is my new webrev: > >> http://cr.openjdk.java.net/~clanger/webrevs/8166584.1/ > >> > >> This looks fine. > >> > >>> What I was thinking a bit more about after I posted my initial webrev was > the > >> fact that the old NET_ThrowSocketException would register a GlobalRef to > >> java/net/SocketException whereas the other, more generic code would > always > >> use the lookup by name. Would you think it is a performance benefit to keep > a > >> reference to a standard exception class in some place and use it for > throwing > >> or is it better to always look up the class? Throwing those kind of > >> exceptions > is > >> probably not on the hot path anyway - but on the other hand it should be no > >> issue to keep references to these very basic class types. What's your view > >> on > >> that? > >> > >> I don’t believe that using a GlobalRef is worth it here. It adds a little > >> complication, while not offering much benefit. JNU_ThrowByName > >> should be fine. > >> > >>> And another probably aesthetic thing: I notice that sometimes a > >> JNU_JAVANETPKG "SocketException" is used and sometimes a > >> "java/net/SocketException", even within the same file like > SocketInputStream.c. > >> Maybe I should unify this in the files that I touch here and if yes, shall > >> I use > the > >> literal name or the JNU_JAVANETPKG define? Any opinion on that? > >> > >> My preference is to remove JNU_JAVANETPKG, and just use "java/net/“. > >> > >> -Chris > >> > >>> Thanks for taking care of this, > >>> Christoph > >>> > >>> > -Original Message- > From: Chris Hegarty [mailto:chris.hega...@oracle.com] > Sent: Montag, 26. September 2016 16:51 > To: Langer, Christoph ; net- > >> d...@openjdk.java.net > Subject: Re: RFR(XS): 8166584: Remove obsolete utility function > NET_ThrowSocketException in windows libnet > > Christoph, > > On 22/09/16 21:59, Langer, Christoph wrote: > > Hi, > > > > while looking at utility functions for creating exceptions in > > libjava/libnet I found a small spot that should be consolidated right > away. > > > > > > The function NET_ThrowSocketException does only exist in the windows > > native implementation and is only used in 3 places in > > SocketInputStream.c. I removed this in favor of directly calling > > JNU_ThrowByName as the Unix variant of that code already does. > > > > > > In that function Java_java_net_SocketInputStream_socketRead0 I also > > replaced throwing a SocketException with throwing an NPE in the rare > > case that a the JNI input for the file descriptor is null. That's > > probably more natural and should virtually never occur anyways. > > Hmmm... I'm not sure about this. SocketException is thrown on > unix too for a similar situation. More significantly, a null value > represents that the socket has been, possibly asynchronously, > closed. > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8166584 > > > > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8166584.0/ > > Other than the above concern, the remainder of the code looks ok > to me. > > -Chris. > >
RFR(S): 8166850: No runtime error expected after calling NET_MapSocketOption
Hi, during my work on exception sites in the JDK, I spotted another minor place that should be fixed. In the Windows native implementations of DualStackPlainDatagramSocketImpl and DualStackPlainSocketImpl, there are calls to evaluate OS API errors after unsuccessful return of NET_MapSocketOption where this should not be done. Please review my fix. I also took the opportunity to clean up some indentations. Bug: https://bugs.openjdk.java.net/browse/JDK-8166850 Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8166850.0/ Thanks Christoph
RE: RFR 8153674: Expected SocketException not thrown when calling bind() with setReuseAddress(false)
Hi Vyom, Thank you very much checking with us. We agree that SO_RESUEADDR and SO_REUSEPORT behave the same way for MulticastSocket. There is no need to check and enable SO_REUSEPORT for this particular type of socket. SO_REUSEADDR is sufficient. Thanks, Lucy From: Vyom Tewari [mailto:vyom.tew...@oracle.com] Sent: Wednesday, September 28, 2016 1:26 AM To: Chris Hegarty ; Mark Sheppard ; net-dev ; Kaczmarek, Eric ; Viswanathan, Sandhya ; Kharbas, Kishor ; Aundhe, Shirish ; Lu, Yingqi Subject: Re: RFR 8153674: Expected SocketException not thrown when calling bind() with setReuseAddress(false) Hi All, I had off line discussion here at Oracle and we decided not to override getReuseAddr/setReuseAddr for MulticastSocket. If user wants, he can set the SO_REUSEPORT with "setOption" before bind. For MulticastSocket SO_REUSEADDR&SO_REUSEPORT are semantically equivalent which means either option will be sufficient to indicate that the address&port is reusable. So setting SO_REUSEPORT in constructor is really necessary/required ? I am looking some comments on this from Intel people(they are in mail chain) who did this original change, before we decide how we wants to proceed on this issue. Thanks, Vyom On Wednesday 14 September 2016 08:47 PM, Chris Hegarty wrote: On 14/09/16 15:53, Mark Sheppard wrote: that's true wrt SO_REUSEPORT in MulticastSocket constructor. But the same could have been argued for the original invocation of setReuseAddress, by default , in the constructors, which is encapsulating, what pereceived as, common or defacto practice wrt applying SO_REUSEADDR on mcast sockets at the system level. As I understand it, it is generally perceived that SO_REUSEADDR and SO_REUSEPORT are semantically equivalent for multicast sockets. As such, I think in the case of MulticastSocket, the fact that the setRuseAddress() is called in the constructor, it is appropriate to set SO_REUSEPORT also when it exists in the OS. I take your point on the semantics of setReuseAddress in DatagramSocket as per its spec. The spec does allude to MulticastSocket. As such, the current proposal's changes just lack the appropriate javadoc to describe its behavior, and its additional functionality of setting SO_REUSEPORT. It is not necessary that overridden method should mirror the semantics of the base class method. If it is accepted that it is generally perceived that SO_REUSEADDR and SO_REUSEPORT are semantically equivalent for multicast sockets, then it seems appropriate that an overriding setReuseAddress(..) method in MulticastSocket can reflect this. That sounds reasonable. -Chris. regards Mark On 14/09/2016 14:58, Chris Hegarty wrote: One additional remark. Was it appropriate to update the legacy MC constructors to set the new JDK 9 SO_REUSEPORT in the first place? This can be achievable, opt-in from new code, by creating an unbound MS, setting the option, then binding. -Chris. On 14/09/16 14:47, Chris Hegarty wrote: Mark, On 14/09/16 14:22, Mark Sheppard wrote: Hi Chris, I don't fully understand your objections to the approach taken. Is there a compatibility issue with the addition of the additional methods to MulticastSocket? The concern is with setReuseAddress performing an operation that is not clear from the method specification, e.g. from setReuseAddress() * Enable/disable the SO_REUSEADDR socket option. This is no longer accurate. The proposed changes would affect SO_REUSEPORT too. I don't see Datagram.setReuseAddress(...) handling the SO_REUSEPORT option, this has to be done explicitly via setOption at this level of abstraction. Yes, it is a bit odd, but these are legacy classes. I am not opposed to adding a new method for this, or something else. I just want to avoid future issues and confusion when setReuseAddress is called and then it is noticed that, the somewhat orthogonal option, SO_REUSEPORT's value has changed. setReuseAddress's spec is very clear about what it does. MulticastSocket is a subclass of DatagramSocket (that in itself is a questionable structuring), and as such has specialized behaviour, and part of that specialization is the setting of the setting SO_REUSEADDR and SO_REUSEPORT in its constructors, to enable address reuse semantics, prior to binding an address. Understood. Of course, the setting of SO_REUSEPORT is new in 9, hence the problem. As part of that specialization, it would seem appropriate that MulticastSocket manipulate the SO_REUSEPORT option in a consistent way. Adding an overridden setReuseAddress(...) method provides that consistency and encapsulates the specialized behaviour. I agree with the abstraction, just not that setReuseAddress should be used to achieve it. The name and spec of this method is so tied to SO_REUSEADDR. Is alternatively proposal to NOT do anything to MulticastSocket, BUT document clearly how to handle the failing scenario, that an MulticastSocket requires both setReuseAddress() and a setOption call
Re: RFR 8153674: Expected SocketException not thrown when calling bind() with setReuseAddress(false)
Thank you Lucy, We’ll proceed with removing the setting of SO_REUSEPORT from the MulticastSocket constructor and spec. -Chris. > On 28 Sep 2016, at 20:02, Lu, Yingqi wrote: > > Hi Vyom, > > Thank you very much checking with us. > > We agree that SO_RESUEADDR and SO_REUSEPORT behave the same way for > MulticastSocket. There is no need to check and enable SO_REUSEPORT for this > particular type of socket. SO_REUSEADDR is sufficient. > > Thanks, > Lucy > > <>From: Vyom Tewari [mailto:vyom.tew...@oracle.com] > Sent: Wednesday, September 28, 2016 1:26 AM > To: Chris Hegarty ; Mark Sheppard > ; net-dev ; Kaczmarek, > Eric ; Viswanathan, Sandhya > ; Kharbas, Kishor ; > Aundhe, Shirish ; Lu, Yingqi > Subject: Re: RFR 8153674: Expected SocketException not thrown when calling > bind() with setReuseAddress(false) > > Hi All, > > I had off line discussion here at Oracle and we decided not to override > getReuseAddr/setReuseAddr for MulticastSocket. If user wants, he can set the > SO_REUSEPORT with "setOption" before bind. > > For MulticastSocket SO_REUSEADDR&SO_REUSEPORT are semantically equivalent > which means either option will be sufficient to indicate that the > address&port is reusable. So setting SO_REUSEPORT in constructor is really > necessary/required ? > > I am looking some comments on this from Intel people(they are in mail chain) > who did this original change, before we decide how we wants to proceed on > this issue. > > Thanks, > > Vyom > > > On Wednesday 14 September 2016 08:47 PM, Chris Hegarty wrote: > On 14/09/16 15:53, Mark Sheppard wrote: > > > that's true wrt SO_REUSEPORT in MulticastSocket constructor. But the > same could have been argued for the original > invocation of setReuseAddress, by default , in the constructors, which > is encapsulating, what pereceived as, common or defacto > practice wrt applying SO_REUSEADDR on mcast sockets at the system level. > As I understand it, it is generally perceived that SO_REUSEADDR and > SO_REUSEPORT are semantically equivalent for multicast sockets. > As such, I think in the case of MulticastSocket, the fact that the > setRuseAddress() is called in the constructor, it is appropriate > to set SO_REUSEPORT also when it exists in the OS. > > I take your point on the semantics of setReuseAddress in DatagramSocket > as per its spec. The spec does allude to MulticastSocket. > As such, the current proposal's changes just lack the appropriate > javadoc to describe its behavior, and its additional functionality of > setting SO_REUSEPORT. > It is not necessary that overridden method should mirror the semantics > of the base class method. > If it is accepted that it is generally perceived that SO_REUSEADDR and > SO_REUSEPORT are semantically equivalent for multicast sockets, > then it seems appropriate that an overriding setReuseAddress(..) method > in MulticastSocket can reflect this. > > That sounds reasonable. > > -Chris. > > > regards > Mark > > > > On 14/09/2016 14:58, Chris Hegarty wrote: > > One additional remark. > > Was it appropriate to update the legacy MC constructors > to set the new JDK 9 SO_REUSEPORT in the first place? > This can be achievable, opt-in from new code, by creating > an unbound MS, setting the option, then binding. > > -Chris. > > On 14/09/16 14:47, Chris Hegarty wrote: > > Mark, > > On 14/09/16 14:22, Mark Sheppard wrote: > > > Hi Chris, > I don't fully understand your objections to the approach taken. > Is there a compatibility issue with the addition of the additional > methods to MulticastSocket? > > The concern is with setReuseAddress performing an operation that > is not clear from the method specification, e.g. from setReuseAddress() > > * Enable/disable the SO_REUSEADDR socket option. > > This is no longer accurate. The proposed changes would affect > SO_REUSEPORT too. > > > I don't see Datagram.setReuseAddress(...) handling the SO_REUSEPORT > option, this has to be done explicitly via setOption at this level of > abstraction. > > Yes, it is a bit odd, but these are legacy classes. I am not opposed > to adding a new method for this, or something else. I just want to > avoid future issues and confusion when setReuseAddress is called and > then it is noticed that, the somewhat orthogonal option, SO_REUSEPORT's > value has changed. setReuseAddress's spec is very clear about what it > does. > > > MulticastSocket is a subclass of DatagramSocket (that in itself is a > questionable structuring), and as such > has specialized behaviour, and part of that specialization is the > setting of the setting SO_REUSEADDR and SO_REUSEPORT > in its constructors, to enable address reuse semantics, prior to > binding > an address. > > Understood. Of course, the setting of SO_REUSEPORT is new in 9, > hence the problem. > > > As part of that specialization, it would seem appropriate that > MulticastSocket m
