Re: RFR: 8269481: SctpMultiChannel never releases own file descriptor [v2]
On Tue, 29 Jun 2021 06:52:47 GMT, Alan Bateman wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8269481: Set state to KILLED *before* closing socket
>
> 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.
-
PR: https://git.openjdk.java.net/jdk/pull/4621
Re: RFR: 8269481: SctpMultiChannel never releases own file descriptor [v2]
> 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: Set state to KILLED *before* closing socket - Changes: - all: https://git.openjdk.java.net/jdk/pull/4621/files - new: https://git.openjdk.java.net/jdk/pull/4621/files/23cdf146..cc18da47 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4621&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4621&range=00-01 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 [v2]
On Tue, 29 Jun 2021 16:10:44 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: Set state to KILLED *before* closing socket
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;
The change here seems mostly benign - there is no reason why either SctpChannel
or SctpServerChannel would find themselves in this state - but it seems ok.
src/jdk.sctp/unix/classes/sun/nio/ch/sctp/SctpMultiChannelImpl.java line 377:
> 375: if (state == ChannelState.UNINITIALIZED) {
> 376: SctpNet.close(fdVal);
> 377: state = ChannelState.KILLED;
Good catch. It seems that there was a missing state here, but not worth adding
at this point.
test/jdk/com/sun/nio/sctp/SctpMultiChannel/CloseDescriptors.java line 71:
> 69: ProcessBuilder pb = new ProcessBuilder(
> 70: "lsof", "-U", "-a", "-p", Long.toString(myPid));
> 71: Process p = pb.start();
Is this test stable? Do we do similar `losf` in other areas ?
-
PR: https://git.openjdk.java.net/jdk/pull/4621
Re: RFR: 8269481: SctpMultiChannel never releases own file descriptor [v2]
On Tue, 29 Jun 2021 16:55:33 GMT, Chris Hegarty wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8269481: Set state to KILLED *before* closing socket
>
> src/jdk.sctp/unix/classes/sun/nio/ch/sctp/SctpMultiChannelImpl.java line 377:
>
>> 375: if (state == ChannelState.UNINITIALIZED) {
>> 376: SctpNet.close(fdVal);
>> 377: state = ChannelState.KILLED;
>
> Good catch. It seems that there was a missing state here, but not worth
> adding at this point.
I also thought that it looks like there is a missing state.
-
PR: https://git.openjdk.java.net/jdk/pull/4621
Re: RFR: 8269481: SctpMultiChannel never releases own file descriptor [v2]
On Tue, 29 Jun 2021 16:58:53 GMT, Chris Hegarty wrote: >> Brian Burkhalter has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8269481: Set state to KILLED *before* closing socket > > test/jdk/com/sun/nio/sctp/SctpMultiChannel/CloseDescriptors.java line 71: > >> 69: ProcessBuilder pb = new ProcessBuilder( >> 70: "lsof", "-U", "-a", "-p", Long.toString(myPid)); >> 71: Process p = pb.start(); > > Is this test stable? Do we do similar `losf` in other areas ? `test/jdk/java/net/DatagramSocket/UnreferencedDatagramSockets.java ` `test/jdk/java/net/MulticastSocket/UnreferencedMulticastSockets.java ` Might want to change this test to be robust as to the location of the `lsof` executable. - PR: https://git.openjdk.java.net/jdk/pull/4621
Re: RFR: 8269481: SctpMultiChannel never releases own file descriptor [v3]
> 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: Change test to look for lsof in more than one location - Changes: - all: https://git.openjdk.java.net/jdk/pull/4621/files - new: https://git.openjdk.java.net/jdk/pull/4621/files/cc18da47..c8865603 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4621&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4621&range=01-02 Stats: 17 lines in 1 file changed: 15 ins; 0 del; 2 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
