RFR 8153674: Expected SocketException not thrown when calling bind() with setReuseAddress(false)

Langer, Christoph christoph.langer at sap.com
Tue Oct 4 08:50:52 UTC 2016 

+1 ... Looks good to me

> -----Original Message-----
> From: net-dev [mailto:net-dev-bounces at openjdk.java.net] On Behalf Of Chris
> Hegarty
> Sent: Donnerstag, 29. September 2016 10:21
> To: Vyom Tewari <vyom.tewari at oracle.com>
> Cc: Kharbas, Kishor <kishor.kharbas at intel.com>; Viswanathan, Sandhya
> <sandhya.viswanathan at intel.com>; Aundhe, Shirish
> <shirish.aundhe at intel.com>; net-dev <net-dev at openjdk.java.net>; Kaczmarek,
> Eric <eric.kaczmarek at intel.com>
> Subject: Re: RFR 8153674: Expected SocketException not thrown when calling
> bind() with setReuseAddress(false)
> 
> 
> > On 29 Sep 2016, at 09:16, Vyom Tewari <vyom.tewari at oracle.com> wrote:
> >
> > Hi All,
> >
> > Please find the latest
> webrev(http://cr.openjdk.java.net/~vtewari/8153674/webrev0.1/index.html). I
> had removed the SO_REUSEPORT & spec from constructor.
> 
> Thank you Vyom, this looks good to me.
> 
> -Chris.
> 
> > Thanks,
> >
> > Vyom
> >
> > On Thursday 29 September 2016 01:38 AM, Chris Hegarty wrote:
> >> 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 <yingqi.lu at intel.com> 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.tewari at oracle.com]
> >>> Sent: Wednesday, September 28, 2016 1:26 AM
> >>> To: Chris Hegarty <chris.hegarty at oracle.com>; Mark Sheppard
> <mark.sheppard at oracle.com>; net-dev <net-dev at openjdk.java.net>;
> Kaczmarek, Eric <eric.kaczmarek at intel.com>; Viswanathan, Sandhya
> <sandhya.viswanathan at intel.com>; Kharbas, Kishor
> <kishor.kharbas at intel.com>; Aundhe, Shirish <shirish.aundhe at intel.com>; Lu,
> Yingqi <yingqi.lu at intel.com>
> >>> 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 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.
> >>>
> >>>
> >>>
> >>> regards
> >>> Mark
> >>>
> >>> On 14/09/2016 13:34, Chris Hegarty wrote:
> >>>
> >>> Mark,
> >>>
> >>> On 14/09/16 13:23, Mark Sheppard wrote:
> >>>
> >>>
> >>> Hi Chris,
> >>>     so are you accepting that it is correct to add the overridden
> >>> methods in MulticastSocket and that these need
> >>> appropriate javadoc ?
> >>>
> >>> I think we need these, but they should just call their super
> >>> equivalents, i.e. no implicit setting of SO_REUSEPORT. They would
> >>> exist solely as a place to locate guidance, or a note, that one
> >>> will likely want to set SO_REUSEPORT too.
> >>>
> >>>
> >>> or are you advocating pushing the handing of the SO_REUSEPORT into
> >>> the
> >>> base DatagramSocket class ?
> >>>
> >>> It is already there. I am not proposing to change this.
> >>>
> >>>
> >>> It is not clear how your code changes fit in the proposed fix i.e.
> >>> the
> >>> explicit setting of the option to false?
> >>>
> >>> My proposal is an alternative. It is not related to the current
> >>> webrev.
> >>>
> >>>
> >>> With the current proposed changes then I think it would be
> >>> sufficient to
> >>> invoke setReuseAddress(true) in MulticastSocket constructors
> >>> rather than
> >>>
> >>>         // Enable SO_REUSEADDR before binding
> >>>         setReuseAddress
> >>> <https://java.se.oracle.com/source/s?defs=setReuseAddress&project=jdk9-
> dev>(*true*);
> >>>
> >>>
> >>>
> >>>
> >>>         // Enable SO_REUSEPORT if supported before binding
> >>>         *if* (supportedOptions
> >>> <https://java.se.oracle.com/source/xref/jdk9-
> dev/jdk/src/java.base/share/classes/java/net/MulticastSocket.java#supportedO
> ptions>().contains
> >>>
> >>>
> >>>
> >>> <https://java.se.oracle.com/source/s?defs=contains&project=jdk9-
> dev>(StandardSocketOptions
> >>>
> >>>
> >>>
> >>>
> <https://java.se.oracle.com/source/s?defs=StandardSocketOptions&project=jdk
> 9-dev>.SO_REUSEPORT
> >>>
> >>>
> >>>
> >>> <https://java.se.oracle.com/source/s?defs=SO_REUSEPORT&project=jdk9-
> dev>))
> >>>
> >>>
> >>> {
> >>>             *this*.setOption
> >>> <https://java.se.oracle.com/source/s?defs=setOption&project=jdk9-
> dev>(StandardSocketOptions
> >>>
> >>>
> >>>
> >>>
> <https://java.se.oracle.com/source/s?defs=StandardSocketOptions&project=jdk
> 9-dev>.SO_REUSEPORT
> >>>
> >>>
> >>>
> >>> <https://java.se.oracle.com/source/s?defs=SO_REUSEPORT&project=jdk9-
> dev>,
> >>>
> >>>
> >>> *true*);
> >>>         }
> >>>
> >>>
> >>> as the overridden setReuseAddress takes care of SO_REUSEPORT
> >>>
> >>> Yes, this is what Vyom has proposed, in the webrev.
> >>>
> >>> I would like to explore an alternative, so see what it would look
> >>> like.
> >>>
> >>> -Chris.
> >>>
> >>>
> >>>
> >>> regards
> >>> Mark
> >>>
> >>> On 14/09/2016 11:43, Chris Hegarty wrote:
> >>>
> >>> Vyom,
> >>>
> >>> On 11/09/16 08:01, Vyom Tewari wrote:
> >>>
> >>> Hi All,
> >>>
> >>> Please review the below code change.
> >>>
> >>> Bug        : https://bugs.openjdk.java.net/browse/JDK-8153674
> >>>
> >>> Webrev  :
> >>> http://cr.openjdk.java.net/~vtewari/8153674/webrev0.0/index.html
> >>> <http://cr.openjdk.java.net/%7Evtewari/8153674/webrev0.0/index.html>
> >>>
> >>>
> >>> This change override the "get/setReuseAddress"  for MulticaseSocket
> >>> and
> >>> will abstract with both reuse attributes(SO_REUSEADDR &
> >>> SO_REUSEPORT).
> >>>
> >>> This issue arises since the changes for 6432031 "Add support for
> >>> SO_REUSEPORT" [1], which sets SO_REUSEPORT on MulticastSocket, if
> >>> the available. So setting setReuseAddress(false) alone is no longer
> >>> sufficient to disable address/port reuse, one must also set
> >>> SO_REUSEPORT to false.
> >>>
> >>> I would be really nervous about changing set/getReuseAddress,
> >>> without
> >>> at least updating the javadoc to indicate that it is now, for MS,
> >>> operating on two socket options.  Although, I do have sympathy
> >>> here since SO_REUSEPORT and SO_REUSEADDR are almost identical when
> >>> dealing with multicasting.
> >>>
> >>> An alternative, maybe; Since the MS constructors document that
> >>> SO_REUSEPORT will be set, where available, maybe a javadoc note
> >>> on the set/getReuseAddress methods would be sufficient, that
> >>> indicates that StandardSocketOptions#SO_REUSEPORT may also need
> >>> to be set to have the desired effect?
> >>>
> >>> If so, then code would have to:
> >>>
> >>>     setReuseAddress(false);
> >>>
> >>>     if
> >>> (supportedOptions().contains(StandardSocketOptions.SO_REUSEPORT))
> >>> this.setOption(StandardSocketOptions.SO_REUSEPORT, false);
> >>>
> >>>   , but at least it is explicit.
> >>>
> >>> Q: not all MS constructors document that SO_REUSEPORT is set, but
> >>> they should, right? This seems to have slipped past during 6432031
> >>> [1].
> >>>
> >>> -Chris.
> >>>
> >>> [1] https://bugs.openjdk.java.net/browse/JDK-6432031
> >>>
> >>>
> >>>
> >>>
> >>
> >

More information about the net-dev mailing list