RFR 8153674: Expected SocketException not thrown when calling bind() with setReuseAddress(false)
Chris Hegarty
chris.hegarty at oracle.com
Thu Sep 29 08:21:10 UTC 2016
> 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#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
>>>
>>>
>>>
>>>
>>
>
More information about the net-dev
mailing list