> On 29 Sep 2016, at 09:16, Vyom Tewari <vyom.tew...@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...@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>(*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>().contains >>> >>> >>> >>> >>> <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>.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=jdk9-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 >>> >>> >>> >>> >> >
