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...@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.tew...@oracle.com] > Sent: Wednesday, September 28, 2016 1:26 AM > To: Chris Hegarty <chris.hega...@oracle.com>; Mark Sheppard > <mark.shepp...@oracle.com>; net-dev <net-dev@openjdk.java.net>; Kaczmarek, > Eric <eric.kaczma...@intel.com>; Viswanathan, Sandhya > <sandhya.viswanat...@intel.com>; Kharbas, Kishor <kishor.khar...@intel.com>; > Aundhe, Shirish <shirish.aun...@intel.com>; Lu, Yingqi <yingqi...@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> > <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#supportedOptions> > > <https://java.se.oracle.com/source/xref/jdk9-dev/jdk/src/java.base/share/classes/java/net/MulticastSocket.java#supportedOptions>().contains > > > > > <https://java.se.oracle.com/source/s?defs=contains&project=jdk9-dev> > <https://java.se.oracle.com/source/s?defs=contains&project=jdk9-dev>(StandardSocketOptions > > > > > <https://java.se.oracle.com/source/s?defs=StandardSocketOptions&project=jdk9-dev> > > <https://java.se.oracle.com/source/s?defs=StandardSocketOptions&project=jdk9-dev>.SO_REUSEPORT > > > > > <https://java.se.oracle.com/source/s?defs=SO_REUSEPORT&project=jdk9-dev> > <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> > <https://java.se.oracle.com/source/s?defs=setOption&project=jdk9-dev>(StandardSocketOptions > > > > > <https://java.se.oracle.com/source/s?defs=StandardSocketOptions&project=jdk9-dev> > > <https://java.se.oracle.com/source/s?defs=StandardSocketOptions&project=jdk9-dev>.SO_REUSEPORT > > > > > <https://java.se.oracle.com/source/s?defs=SO_REUSEPORT&project=jdk9-dev> > <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 > <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> > <http://cr.openjdk.java.net/%7Evtewari/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 > <https://bugs.openjdk.java.net/browse/JDK-6432031> > > > >
