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
