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

[Chris Hegarty]

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


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

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

[Mark Sheppard]

Hi Chris,
so are you accepting that it is correct to add the overridden 
methods in MulticastSocket and that these need

appropriate javadoc ?

or are you advocating pushing the handing of the SO_REUSEPORT into the 
base DatagramSocket class ?


It is not clear how your code changes fit in the proposed fix i.e. the 
explicit setting of the option to false?


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 
(*true*);


// Enable SO_REUSEPORT if supported before binding
*if*  (supportedOptions 
().contains 
(StandardSocketOptions 
.SO_REUSEPORT 
)) {
*this*.setOption 
(StandardSocketOptions 
.SO_REUSEPORT 
,*true*);

}


as the overridden setReuseAddress takes care of SO_REUSEPORT

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


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

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

[Chris Hegarty]

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
(*true*);

// Enable SO_REUSEPORT if supported before binding
*if* (supportedOptions
().contains
(StandardSocketOptions
.SO_REUSEPORT
)) {
*this*.setOption
(StandardSocketOptions
.SO_REUSEPORT
, 
*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


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

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

[Mark Sheppard]

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?


I don't see Datagram.setReuseAddress(...) handling the SO_REUSEPORT 
option, this has to be done explicitly via setOption at this level of 
abstraction.


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

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 ?



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

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
(*true*); 



// Enable SO_REUSEPORT if supported before binding
*if* (supportedOptions
().contains 

(StandardSocketOptions 

.SO_REUSEPORT 

)) 
{

*this*.setOption
(StandardSocketOptions 

.SO_REUSEPORT 

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


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

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

[Chris Hegarty]

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
(*true*);


// Enable SO_REUSEPORT if supported before binding
*if* (supportedOptions
().contains

(StandardSocketOptions

.SO_REUSEPORT

))
{
*this*.setOption
(StandardSocketOptions

.SO_REUSEPORT

,
*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.ope

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

[Chris Hegarty]

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
(*true*);



// Enable SO_REUSEPORT if supported before binding
*if* (supportedOptions
().contains


(StandardSocketOptions


.SO_REUSEPORT


))

{
*this*.setOption
(StandardSocketOptions


.SO_REUSEPORT


,

*true*);
}


as the overridden setReuseAddress takes care of SO_REUSEPORT


Yes, this is what Vyom has proposed, in the webrev.

I would like to expl

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

[Mark Sheppard]

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 fal

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

[Chris Hegarty]

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

RFR - 8165988: Test JarURLConnectionUseCaches.java fails at windows: failed to clean up files after test

[Rob McKenna]

Hi folks,

A resource cleanup issue can cause this test to fail on windows, fix is to run 
in othervm:

http://cr.openjdk.java.net/~robm/8165988/webrev.01/

-Rob

Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

[Artem Smotrakov]

Not urgent, but I would appreciate if someone can get a chance to look 
at this.


Artem


On 09/07/2016 03:17 PM, Artem Smotrakov wrote:

Sending to net-dev@openjdk.java.net as well.

Artem


On 09/07/2016 12:28 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for 
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java


The test has been observed to fail a couple of times, but it's still 
not clear why it failed because there is not much info in logs. The 
patch updates the test to enable additional debug output, so that we 
have more info if it fails next time.


While looking at the test, I notices a couple of issues, but they 
don't seem to cause these intermittent failures:
- The test sets system properties for JSSE in a loop, but JSSE 
provider reads them only once while initialization. As a result, only 
values which were set in the first iteration are actually used.

- The test doesn't close files and sockets sometimes.

The patch also fixed the issues above, and there are a couple 
cosmetic changes.


Bug: https://bugs.openjdk.java.net/browse/JDK-8164591
Webrev: http://cr.openjdk.java.net/~asmotrak/8164591/webrev.00/

Artem

Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

[Xuelei Fan]

As you were already there, I would suggest to consider the 
SSLSocketSample.java template as the comment in JDK-8163924 review thread.


Thanks,
Xuelei

On 9/15/2016 9:13 AM, Artem Smotrakov wrote:

Not urgent, but I would appreciate if someone can get a chance to look
at this.

Artem


On 09/07/2016 03:17 PM, Artem Smotrakov wrote:

Sending to net-dev@openjdk.java.net as well.

Artem


On 09/07/2016 12:28 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java

The test has been observed to fail a couple of times, but it's still
not clear why it failed because there is not much info in logs. The
patch updates the test to enable additional debug output, so that we
have more info if it fails next time.

While looking at the test, I notices a couple of issues, but they
don't seem to cause these intermittent failures:
- The test sets system properties for JSSE in a loop, but JSSE
provider reads them only once while initialization. As a result, only
values which were set in the first iteration are actually used.
- The test doesn't close files and sockets sometimes.

The patch also fixed the issues above, and there are a couple
cosmetic changes.

Bug: https://bugs.openjdk.java.net/browse/JDK-8164591
Webrev: http://cr.openjdk.java.net/~asmotrak/8164591/webrev.00/

Artem

Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

[Artem Smotrakov]

Hi Xuelei,

For this one, I am not sure that it would help here since the test 
failed after it already had started handshaking. I would prefer to have 
it as a separate enhancement.


Artem


On 09/14/2016 06:19 PM, Xuelei Fan wrote:
As you were already there, I would suggest to consider the 
SSLSocketSample.java template as the comment in JDK-8163924 review 
thread.


Thanks,
Xuelei

On 9/15/2016 9:13 AM, Artem Smotrakov wrote:

Not urgent, but I would appreciate if someone can get a chance to look
at this.

Artem


On 09/07/2016 03:17 PM, Artem Smotrakov wrote:

Sending to net-dev@openjdk.java.net as well.

Artem


On 09/07/2016 12:28 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java

The test has been observed to fail a couple of times, but it's still
not clear why it failed because there is not much info in logs. The
patch updates the test to enable additional debug output, so that we
have more info if it fails next time.

While looking at the test, I notices a couple of issues, but they
don't seem to cause these intermittent failures:
- The test sets system properties for JSSE in a loop, but JSSE
provider reads them only once while initialization. As a result, only
values which were set in the first iteration are actually used.
- The test doesn't close files and sockets sometimes.

The patch also fixed the issues above, and there are a couple
cosmetic changes.

Bug: https://bugs.openjdk.java.net/browse/JDK-8164591
Webrev: http://cr.openjdk.java.net/~asmotrak/8164591/webrev.00/

Artem

Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

[Xuelei Fan]

On 9/15/2016 9:23 AM, Artem Smotrakov wrote:

Hi Xuelei,

For this one, I am not sure that it would help here since the test
failed after it already had started handshaking.
It has the same issue as a free-port is used.  We don't actually know 
the handshake is coming from the right client.


Xuelei


I would prefer to have it as a separate enhancement.

Artem


On 09/14/2016 06:19 PM, Xuelei Fan wrote:

As you were already there, I would suggest to consider the
SSLSocketSample.java template as the comment in JDK-8163924 review
thread.

Thanks,
Xuelei

On 9/15/2016 9:13 AM, Artem Smotrakov wrote:

Not urgent, but I would appreciate if someone can get a chance to look
at this.

Artem


On 09/07/2016 03:17 PM, Artem Smotrakov wrote:

Sending to net-dev@openjdk.java.net as well.

Artem


On 09/07/2016 12:28 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java

The test has been observed to fail a couple of times, but it's still
not clear why it failed because there is not much info in logs. The
patch updates the test to enable additional debug output, so that we
have more info if it fails next time.

While looking at the test, I notices a couple of issues, but they
don't seem to cause these intermittent failures:
- The test sets system properties for JSSE in a loop, but JSSE
provider reads them only once while initialization. As a result, only
values which were set in the first iteration are actually used.
- The test doesn't close files and sockets sometimes.

The patch also fixed the issues above, and there are a couple
cosmetic changes.

Bug: https://bugs.openjdk.java.net/browse/JDK-8164591
Webrev: http://cr.openjdk.java.net/~asmotrak/8164591/webrev.00/

Artem

Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

[Artem Smotrakov]

Well, in this particular case it's not clear that it has the same issue 
with free port (at least to me). The exception occurred on client side, 
so it's not the case where we don't know where the handshake came from.


I can make this enhancement, but like I said I don't think it's going to 
help, so I would like to keep debug output on.


Artem


On 09/14/2016 06:39 PM, Xuelei Fan wrote:

On 9/15/2016 9:23 AM, Artem Smotrakov wrote:

Hi Xuelei,

For this one, I am not sure that it would help here since the test
failed after it already had started handshaking.
It has the same issue as a free-port is used.  We don't actually know 
the handshake is coming from the right client.


Xuelei


I would prefer to have it as a separate enhancement.

Artem


On 09/14/2016 06:19 PM, Xuelei Fan wrote:

As you were already there, I would suggest to consider the
SSLSocketSample.java template as the comment in JDK-8163924 review
thread.

Thanks,
Xuelei

On 9/15/2016 9:13 AM, Artem Smotrakov wrote:

Not urgent, but I would appreciate if someone can get a chance to look
at this.

Artem


On 09/07/2016 03:17 PM, Artem Smotrakov wrote:

Sending to net-dev@openjdk.java.net as well.

Artem


On 09/07/2016 12:28 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java

The test has been observed to fail a couple of times, but it's still
not clear why it failed because there is not much info in logs. The
patch updates the test to enable additional debug output, so that we
have more info if it fails next time.

While looking at the test, I notices a couple of issues, but they
don't seem to cause these intermittent failures:
- The test sets system properties for JSSE in a loop, but JSSE
provider reads them only once while initialization. As a result, 
only

values which were set in the first iteration are actually used.
- The test doesn't close files and sockets sometimes.

The patch also fixed the issues above, and there are a couple
cosmetic changes.

Bug: https://bugs.openjdk.java.net/browse/JDK-8164591
Webrev: http://cr.openjdk.java.net/~asmotrak/8164591/webrev.00/

Artem

Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

[Xuelei Fan]

On 9/15/2016 9:45 AM, Artem Smotrakov wrote:

Well, in this particular case it's not clear that it has the same issue
with free port (at least to me). The exception occurred on client side,
so it's not the case where we don't know where the handshake came from.

;-) Yeh, you catch the point.  But there is a free-port issue although 
the exception stack in the bug description may be not that case.


Let's look at a scenarios:
1. server open a server socket and listen.
2. other test case connect to the server socket.
3. this test case try to connect to server socket.
4. this test case would fail as the server only accept one connections.

I did not check it very carefully, I think for #4, the exception stack 
can be similar to the one in the bug description.


Anyway, as a free port is used, there are free-port issues.  Please 
consider to make the enhancement in the fix.  Otherwise, you cannot 
avoid the intermittent failure for this test case in the current testing 
environment.


Xuelei


I can make this enhancement, but like I said I don't think it's going to
help, so I would like to keep debug output on.

Artem


On 09/14/2016 06:39 PM, Xuelei Fan wrote:

On 9/15/2016 9:23 AM, Artem Smotrakov wrote:

Hi Xuelei,

For this one, I am not sure that it would help here since the test
failed after it already had started handshaking.

It has the same issue as a free-port is used.  We don't actually know
the handshake is coming from the right client.

Xuelei


I would prefer to have it as a separate enhancement.

Artem


On 09/14/2016 06:19 PM, Xuelei Fan wrote:

As you were already there, I would suggest to consider the
SSLSocketSample.java template as the comment in JDK-8163924 review
thread.

Thanks,
Xuelei

On 9/15/2016 9:13 AM, Artem Smotrakov wrote:

Not urgent, but I would appreciate if someone can get a chance to look
at this.

Artem


On 09/07/2016 03:17 PM, Artem Smotrakov wrote:

Sending to net-dev@openjdk.java.net as well.

Artem


On 09/07/2016 12:28 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java

The test has been observed to fail a couple of times, but it's still
not clear why it failed because there is not much info in logs. The
patch updates the test to enable additional debug output, so that we
have more info if it fails next time.

While looking at the test, I notices a couple of issues, but they
don't seem to cause these intermittent failures:
- The test sets system properties for JSSE in a loop, but JSSE
provider reads them only once while initialization. As a result,
only
values which were set in the first iteration are actually used.
- The test doesn't close files and sockets sometimes.

The patch also fixed the issues above, and there are a couple
cosmetic changes.

Bug: https://bugs.openjdk.java.net/browse/JDK-8164591
Webrev: http://cr.openjdk.java.net/~asmotrak/8164591/webrev.00/

Artem