Re: [8u-dev]: Request for Review and Approval: 8075484: SocketInputStream.socketRead0 can hang even with soTimeout set
Hi Christoph, > On 9 Jan 2017, at 05:56, Langer, Christoph wrote: > > Ping: Please review this backport to JDK8. > > From: Langer, Christoph > Sent: Donnerstag, 29. Dezember 2016 10:37 > To: net-dev@openjdk.java.net; jdk8u-...@openjdk.java.net > Subject: [8u-dev]: Request for Review and Approval: 8075484: > SocketInputStream.socketRead0 can hang even with soTimeout set > > Hi, > > please review (and eventually approve) the change for downporting 8075484. > > Webrev for 8u-dev: http://cr.openjdk.java.net/~clanger/webrevs/8075484.8udev/ The changes look ok to me. > Bug: https://bugs.openjdk.java.net/browse/JDK-8075484 > JDK9 Change: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/af17b6bc08dd > JDK9 Review Thread(s): > http://mail.openjdk.java.net/pipermail/net-dev/2016-August/010171.html >http://mail.openjdk.java.net/pipermail/net-dev/2016-September/010201.html > > We had customer reports who ran into that issue with Java 8. So this should > be downported. > > The problem is, that the fix does not apply to Solaris as Solaris needs some > calls into hotspot. This is because in JDK 8 the flag for interruptible IO is > still supported (though deprecated). But I think it is still worthwile to > bring this down for the other platforms which I’m proposing with my > changeset. So I extracted the new code manually from the JDK9 changeset and > made it fit into JDK8 coding. Ok, I see the complication. On Solaris these calls still go through the VM to support Interruptible IO ( through the JVM_XXX interface). Thankfully, this is no longer the case in 9. -Chris.
[8u-dev]: Request for Approval: 8075484: SocketInputStream.socketRead0 can hang even with soTimeout set
Hi, please approve this downport. Thanks, Chris, for the Review. Best regards Christoph > -Original Message- > From: Chris Hegarty [mailto:chris.hega...@oracle.com] > Sent: Mittwoch, 11. Januar 2017 11:47 > To: Langer, Christoph > Cc: net-dev@openjdk.java.net > Subject: Re: [8u-dev]: Request for Review and Approval: 8075484: > SocketInputStream.socketRead0 can hang even with soTimeout set > > Hi Christoph, > > > On 9 Jan 2017, at 05:56, Langer, Christoph > wrote: > > > > Ping: Please review this backport to JDK8. > > > > From: Langer, Christoph > > Sent: Donnerstag, 29. Dezember 2016 10:37 > > To: net-dev@openjdk.java.net; jdk8u-...@openjdk.java.net > > Subject: [8u-dev]: Request for Review and Approval: 8075484: > SocketInputStream.socketRead0 can hang even with soTimeout set > > > > Hi, > > > > please review (and eventually approve) the change for downporting 8075484. > > > > Webrev for 8u-dev: > http://cr.openjdk.java.net/~clanger/webrevs/8075484.8udev/ > > The changes look ok to me. > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8075484 > > JDK9 Change: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/af17b6bc08dd > > JDK9 Review Thread(s): > > http://mail.openjdk.java.net/pipermail/net-dev/2016-August/010171.html > >http://mail.openjdk.java.net/pipermail/net-dev/2016- > September/010201.html > > > > We had customer reports who ran into that issue with Java 8. So this should > be downported. > > > > The problem is, that the fix does not apply to Solaris as Solaris needs some > calls into hotspot. This is because in JDK 8 the flag for interruptible IO is > still > supported (though deprecated). But I think it is still worthwile to bring this > down for the other platforms which I’m proposing with my changeset. So I > extracted the new code manually from the JDK9 changeset and made it fit into > JDK8 coding. > > Ok, I see the complication. On Solaris these calls still go through > the VM to support Interruptible IO ( through the JVM_XXX interface). > Thankfully, this is no longer the case in 9. > > -Chris.
Re: [8u-dev]: Request for Approval: 8075484: SocketInputStream.socketRead0 can hang even with soTimeout set
approved for push to 8u-dev once appropriate noreg label has been added to the bug report [ noreg bug labels ] http://openjdk.java.net/guide/changePlanning.html#noreg Cheers, -Buck On 2017/01/11 22:28, Langer, Christoph wrote: Hi, please approve this downport. Thanks, Chris, for the Review. Best regards Christoph -Original Message- From: Chris Hegarty [mailto:chris.hega...@oracle.com] Sent: Mittwoch, 11. Januar 2017 11:47 To: Langer, Christoph Cc: net-dev@openjdk.java.net Subject: Re: [8u-dev]: Request for Review and Approval: 8075484: SocketInputStream.socketRead0 can hang even with soTimeout set Hi Christoph, On 9 Jan 2017, at 05:56, Langer, Christoph wrote: Ping: Please review this backport to JDK8. From: Langer, Christoph Sent: Donnerstag, 29. Dezember 2016 10:37 To: net-dev@openjdk.java.net; jdk8u-...@openjdk.java.net Subject: [8u-dev]: Request for Review and Approval: 8075484: SocketInputStream.socketRead0 can hang even with soTimeout set Hi, please review (and eventually approve) the change for downporting 8075484. Webrev for 8u-dev: http://cr.openjdk.java.net/~clanger/webrevs/8075484.8udev/ The changes look ok to me. Bug: https://bugs.openjdk.java.net/browse/JDK-8075484 JDK9 Change: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/af17b6bc08dd JDK9 Review Thread(s): http://mail.openjdk.java.net/pipermail/net-dev/2016-August/010171.html http://mail.openjdk.java.net/pipermail/net-dev/2016- September/010201.html We had customer reports who ran into that issue with Java 8. So this should be downported. The problem is, that the fix does not apply to Solaris as Solaris needs some calls into hotspot. This is because in JDK 8 the flag for interruptible IO is still supported (though deprecated). But I think it is still worthwile to bring this down for the other platforms which I’m proposing with my changeset. So I extracted the new code manually from the JDK9 changeset and made it fit into JDK8 coding. Ok, I see the complication. On Solaris these calls still go through the VM to support Interruptible IO ( through the JVM_XXX interface). Thankfully, this is no longer the case in 9. -Chris.
RE: RFR:8170868: DefaultProxySelector should use system defaults on Windows, MacOS and Gnome
Hi Chris, I have addressed your comments in a new webrev. Can you please have a look at this one? http://cr.openjdk.java.net/~clanger/webrevs/8170868.3/ I created src/java.base/share/native/libnet/proxy_util.c/h and these files contain now the common used native parts. And I rewrote the code to return an array of Proxy objects from native code - so I do no longer call back to Java to add an object to the list. Best regards, Arno >-Original Message- >From: Chris Hegarty [mailto:chris.hega...@oracle.com] >Sent: Dienstag, 3. Januar 2017 15:04 >To: Zeller, Arno >Cc: net-dev@openjdk.java.net >Subject: Re: RFR:8170868: DefaultProxySelector should use system defaults >on Windows, MacOS and Gnome > >Arno, > >> On 27 Dec 2016, at 11:44, Zeller, Arno wrote: >> >> Hi Chris, >> >> Thanks for having a look at my change: >> >>> 1) It seems awful to have to deal with LinkedList in native code. How >>> about returning an array from native, and then converting that into >>> whatever list type is appropriate at the Java level. >> With the current implementation on Mac I do not know upfront how many >items the list will contain and therefore I preferred to use the LinkedList >from >native code. >> I can of course change the implementation on Mac to first generate a fully >resolved native list and then I can use NewObjectArray to generate an Array >of Proxy objects to return. This will avoid calling to java to add an element >to >the list. Do you think this will be better? > >Yes. Thanks. > >>> 2) I would prefer the use of List.of(...), and list.of() for empty, since >>> these are immutable and efficient list implementations. >> I thought about that but I tried to not return an immutable List because the >Javadoc of Proxy.select does not state anything about the returned List (if it >is >immutable or not) and I feared to break compatibility by returning an >immutable List. >> If you think this is no problem I will of course prefer to always return the >same immutable NO_PROXY list object. > >I would prefer an immutable list. Maybe we file, a separate, issue to amend >the spec to say "returns an immutable list …”. > >>> 3) Is the comment “Inspired ...” necessary / appropriate? >> I will change it to the comment proposed by Volker. > >Ok. > >>> 4) Can some of the native initialization code be moved to a platform >>> independent location, to remove duplication? >> Would it be ok if I move the definition of the static variables and the >implementation of 'static int initJavaClass(...)' to a header file in the >shared >branch. I.e. src/java.base/shared/native/libnet/DefaultProxySelector.h and >include this in the MacOSX, Windows and the Unix implementations? > >Yes. > >>> 5) The new file has a shared copyright header. I see similar SAP >>> headers in a few files, but none shared with the Oracle header. >>> How did you arrive at this format? >> The format was suggested to me by Volker :-) > >Ok. > >-Chris.
RE: [8u-dev]: Request for Approval: 8075484: SocketInputStream.socketRead0 can hang even with soTimeout set
Thanks Buck. I added the label and pushed: http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/c43400eba8a3 I also added #include to net_util_md.c which I forgot in my webrev. Best regards Christoph > -Original Message- > From: David Buck [mailto:david.b...@oracle.com] > Sent: Mittwoch, 11. Januar 2017 14:51 > To: Langer, Christoph ; jdk8u- > d...@openjdk.java.net > Cc: net-dev@openjdk.java.net > Subject: Re: [8u-dev]: Request for Approval: 8075484: > SocketInputStream.socketRead0 can hang even with soTimeout set > > approved for push to 8u-dev once appropriate noreg label has been added > to the bug report > > [ noreg bug labels ] > http://openjdk.java.net/guide/changePlanning.html#noreg > > Cheers, > -Buck > > On 2017/01/11 22:28, Langer, Christoph wrote: > > Hi, > > > > please approve this downport. > > > > Thanks, Chris, for the Review. > > > > Best regards > > Christoph > > > >> -Original Message- > >> From: Chris Hegarty [mailto:chris.hega...@oracle.com] > >> Sent: Mittwoch, 11. Januar 2017 11:47 > >> To: Langer, Christoph > >> Cc: net-dev@openjdk.java.net > >> Subject: Re: [8u-dev]: Request for Review and Approval: 8075484: > >> SocketInputStream.socketRead0 can hang even with soTimeout set > >> > >> Hi Christoph, > >> > >>> On 9 Jan 2017, at 05:56, Langer, Christoph > >> wrote: > >>> > >>> Ping: Please review this backport to JDK8. > >>> > >>> From: Langer, Christoph > >>> Sent: Donnerstag, 29. Dezember 2016 10:37 > >>> To: net-dev@openjdk.java.net; jdk8u-...@openjdk.java.net > >>> Subject: [8u-dev]: Request for Review and Approval: 8075484: > >> SocketInputStream.socketRead0 can hang even with soTimeout set > >>> > >>> Hi, > >>> > >>> please review (and eventually approve) the change for downporting > 8075484. > >>> > >>> Webrev for 8u-dev: > >> http://cr.openjdk.java.net/~clanger/webrevs/8075484.8udev/ > >> > >> The changes look ok to me. > >> > >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8075484 > >>> JDK9 Change: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/af17b6bc08dd > >>> JDK9 Review Thread(s): > >>>http://mail.openjdk.java.net/pipermail/net-dev/2016- > August/010171.html > >>> http://mail.openjdk.java.net/pipermail/net-dev/2016- > >> September/010201.html > >>> > >>> We had customer reports who ran into that issue with Java 8. So this > >>> should > >> be downported. > >>> > >>> The problem is, that the fix does not apply to Solaris as Solaris needs > >>> some > >> calls into hotspot. This is because in JDK 8 the flag for interruptible IO > >> is still > >> supported (though deprecated). But I think it is still worthwile to bring > >> this > >> down for the other platforms which I’m proposing with my changeset. So I > >> extracted the new code manually from the JDK9 changeset and made it fit > into > >> JDK8 coding. > >> > >> Ok, I see the complication. On Solaris these calls still go through > >> the VM to support Interruptible IO ( through the JVM_XXX interface). > >> Thankfully, this is no longer the case in 9. > >> > >> -Chris. > >
RE: RFR: 8170544: Fix code scan findings in libnet
Hi Chris, thanks for looking. > 1) NetworkInterface.c > > I’m not sure that the close is really necessary, since a JNI pending > exception can only be thrown is sock is less than 0. I think just > removing the ' && (*env)->ExceptionOccurred(env)’ from the original > if statement should be sufficient, no? I think you are right. An exception can only occur if socket is less than 0. There is one case where socket could be less than 0 and no exception occurred. In that case we'd probably see an exception later on. But I think it would be fine to return NULL in case socket is < 0. Generally, this coding needs to be revisited in order to make it work for IPv6 only systems. We should do that when finishing up bug 8148424 [1]. > 2) net_util.c > > getInet6Address_scopeid_set should CHECK_NULL_RETURN(holder, JNI_FALSE)? > getInet6Address_scopeid now returns an unsigned in, why > CHECK_NULL_RETURN(holder, -1)? > > Some of this, existing, code seems a little dubious. > Good catch. The CHECK_NULL_RETURN macros need adaption. The reason why getInet6Address_scopeid should return unsigned int is that the struct sockaddr_in6 is also using unsigned int for the scope, e.g. on Linux [2] or Windows [3]. I've addressed your points in http://cr.openjdk.java.net/~clanger/webrevs/8170544.2/ Would you want to run this through JPRT again? I would go and push it towards the end of the week. Best regards Christoph [1] https://bugs.openjdk.java.net/browse/JDK-8148424 [2] http://man7.org/linux/man-pages/man7/ipv6.7.html [3] https://msdn.microsoft.com/en-us/library/windows/hardware/ff570824(v=vs.85).aspx
