RFR 8153674: Expected SocketException not thrown when calling bind() with setReuseAddress(false)
Mark Sheppard
mark.sheppard at oracle.com
Wed Sep 14 14:53:57 UTC 2016
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.
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