RFR 8153674: Expected SocketException not thrown when calling bind() with setReuseAddress(false)
Chris Hegarty
chris.hegarty at oracle.com
Wed Sep 28 20:08:21 UTC 2016
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> <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>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20160928/4f74ebf2/attachment-0001.html>
More information about the net-dev
mailing list