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

Chris Hegarty chris.hegarty at oracle.com
Wed Sep 14 15:17:28 UTC 2016 

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