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

Chris Hegarty chris.hegarty at oracle.com
Wed Sep 28 09:12:57 UTC 2016 

Thanks Vyom,

On 28/09/16 09:25, Vyom Tewari wrote:
> 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.

Right. All options that we looked at seem less than satisfactory
( these getter/setter methods work at a single socket option level ).

> 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 ?

This is the crux of the issue. I have been unable to find the
reasoning behind this particular part of the change for 6432031
[1]. As you correctly say, SO_REUSEADDR and SO_REUSEPORT are
semantically equivalent for multicasting.

> 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.

At this point, without further clarification of why SO_REUSEPORT
was added to the MulticastSocket constructor, my preference is
to revert it.

-Chris.

[1] https://bugs.openjdk.java.net/browse/JDK-6432031

> 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