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

Wed Sep 14 13:58:43 UTC 2016

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