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

Wed Sep 14 13:47:15 UTC 2016

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