Re: RFR: 8269481: SctpMultiChannel never releases own file descriptor [v3]

2021-07-01 Thread Alan Bateman 
On Tue, 29 Jun 2021 16:06:52 GMT, Brian Burkhalter  wrote:

>> src/jdk.sctp/unix/classes/sun/nio/ch/sctp/SctpChannelImpl.java line 643:
>> 
>>> 641: if (state == ChannelState.UNINITIALIZED) {
>>> 642: SctpNet.close(fdVal);
>>> 643: state = ChannelState.KILLED;
>> 
>> It might be better to invert these, meaning set the state to KILLED before 
>> close(fdVal), just in case the close throws.
>
> So modified.

It looks like you've changed some but not all cases. I realize 
ChannelState.UNINITIALIZED is not too interesting but further maintainers may 
wonder about it.

-

PR: https://git.openjdk.java.net/jdk/pull/4621

Integrated: 8268960: com/sun/net/httpserver/Headers.java: Ensure mutators normalize keys and disallow null for keys and values

2021-07-01 Thread Julia Boes 
On Fri, 18 Jun 2021 10:43:06 GMT, Julia Boes  wrote:

> `com.sun.net.httpserver.Headers` normalizes its keys to adhere to the 
> following format: First character uppercase, all other characters lowercase, 
> for example `"foo" -> "Foo"`. This behaviour is not consistent across the 
> mutator methods of the class, in particular `putAll()` and `replaceAll()` do 
> not apply normalization.
> 
> The suggested fix is to update the implementation of `putAll()` and to add an 
> implementation of of the java.util.Map default method `replaceAll()`. While 
> here, we can add a `toString()` implementation to provide a more informative 
> string.
> 
> Additionally,  the Headers class is updated to disallow null values for keys 
> and values.
> 
> Testing: tier 1-3 all clear
> CSR: https://bugs.openjdk.java.net/browse/JDK-8269296

This pull request has now been integrated.

Changeset: 82bfc5d4
Author:Julia Boes 
URL:   
https://git.openjdk.java.net/jdk/commit/82bfc5d45c54fb37dc021bc91fa17efe34f77f44
Stats: 345 lines in 2 files changed: 312 ins; 13 del; 20 mod

8268960: com/sun/net/httpserver/Headers.java: Ensure mutators normalize keys 
and disallow null for keys and values

Reviewed-by: chegar, dfuchs, michaelm

-

PR: https://git.openjdk.java.net/jdk/pull/4528

Re: RFR: 8269481: SctpMultiChannel never releases own file descriptor [v3]

2021-07-01 Thread Brian Burkhalter 


On Jul 1, 2021, at 1:17 AM, Alan Bateman 
mailto:al...@openjdk.java.net>> wrote:

It looks like you've changed some but not all cases. I realize 
ChannelState.UNINITIALIZED is not too interesting but further maintainers may 
wonder about it.

I’m not seeing that I missed any cases. I don’t really want to get into 
modifying the states in this PR.

Re: RFR: 8269481: SctpMultiChannel never releases own file descriptor [v3]

2021-07-01 Thread Brian Burkhalter 
Oh I see now. Thanks.

On Jul 1, 2021, at 8:56 AM, Alan Bateman 
mailto:alan.bate...@oracle.com>> wrote:

I should have been clearer. Look at SctpServerChannelImpl.java L284 and 
SctpMultiChannelImpl.java L376.  SctpNet.close(fdVal) added with the patch 
added them them before setting the state. The suggestion in the previous 
comments was to insert the close after setting the state to KILLED in case the 
close fail (in some corner case).

Re: RFR: 8269481: SctpMultiChannel never releases own file descriptor [v3]

2021-07-01 Thread Alan Bateman 

On 01/07/2021 16:47, Brian Burkhalter wrote:



On Jul 1, 2021, at 1:17 AM, Alan Bateman > wrote:


It looks like you've changed some but not all cases. I realize 
ChannelState.UNINITIALIZED is not too interesting but further 
maintainers may wonder about it.


I’m not seeing that I missed any cases. I don’t really want to get 
into modifying the states in this PR.


I should have been clearer. Look at SctpServerChannelImpl.java L284 and 
SctpMultiChannelImpl.java L376.  SctpNet.close(fdVal) added with the 
patch added them them before setting the state. The suggestion in the 
previous comments was to insert the close after setting the state to 
KILLED in case the close fail (in some corner case).


-Alan

Re: RFR: 8269481: SctpMultiChannel never releases own file descriptor [v4]

2021-07-01 Thread Brian Burkhalter 
> Please review this change to the Unix implementations of 
> `sun.nio.ch.sctp.Sctp*ChannelImpl#kill()` to close the socket if `state == 
> ChannelState.UNINITIALIZED`.

Brian Burkhalter has updated the pull request incrementally with one additional 
commit since the last revision:

  8269481: Fix incorrect order of setting state to KILLED and closing the socket

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4621/files
  - new: https://git.openjdk.java.net/jdk/pull/4621/files/c8865603..676c0f0a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4621&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4621&range=02-03

  Stats: 6 lines in 3 files changed: 3 ins; 3 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4621.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4621/head:pull/4621

PR: https://git.openjdk.java.net/jdk/pull/4621

Re: RFR: 8269481: SctpMultiChannel never releases own file descriptor [v4]

2021-07-01 Thread Alan Bateman 
On Thu, 1 Jul 2021 18:09:39 GMT, Brian Burkhalter  wrote:

>> Please review this change to the Unix implementations of 
>> `sun.nio.ch.sctp.Sctp*ChannelImpl#kill()` to close the socket if `state == 
>> ChannelState.UNINITIALIZED`.
>
> Brian Burkhalter has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8269481: Fix incorrect order of setting state to KILLED and closing the 
> socket

The implementation changes in the latest round look fine.

I didn't study the test too closely I wonder about the reliability of using a 
hard coded port (12345) and using losf on an agent VM. I think it will 
minimally need to run in /othervm mode to avoid shimmer in the numbest of open 
file descriptors that might arise due to the tests that ran previously in the 
same VM.

-

Marked as reviewed by alanb (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4621