Re: RFR: 8170544: Fix code scan findings in libnet
> On 29 Dec 2016, at 13:20, Langer, Christoph wrote: > > Hi Goetz, > > thanks for reviewing this. > > I have addressed your comments in a new webrev: > http://cr.openjdk.java.net/~clanger/webrevs/8170544.1/ This mainly looks fine. Just a few comments: 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? 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. -Chris.
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: RFR[9] JDK-8170641: sun/net/www/protocol/https/HttpsURLConnection/PostThruProxy.sh fails with timeout
On 27 Dec 2016, at 06:24, John Jiang wrote: > > Hi, > sun/net/www/protocol/https/HttpsURLConnection/PostThruProxy.sh failed with > timeout. > The client side could hang if the server goes to timeout before getting the > client request, and the proxy also could be blocked. > > This patch sets timeout for the server and the client and catches > SocketTimeoutException to ignore the . And it removes the usage on shell > script. > The patch also modifies test > sun/net/www/protocol/https/HttpsURLConnection/PostThruProxyWithAuth.sh as > this test has the similar code pattern and issue. > > Webrev: http://cr.openjdk.java.net/~jjiang/8170641/webrev.00/ > Issue: https://bugs.openjdk.java.net/browse/JDK-8170641 Looks good John. Thanks, -Chris.
RFR: 8170785: Excessive allocation in ParseUtil.encodePath
Hi, some users reports high allocation rates in ParseUtil.encodePath, regardless of whether paths actually need to be encoded or not. Since this is a commonly used utility it makes sense. Webrev: http://cr.openjdk.java.net/~redestad/8170785/webrev.01 Bug: https://bugs.openjdk.java.net/browse/JDK-8170785 This patch provides a semantically neutral fast-path for cases when the path does not need to be encoded (up to 5x speedup), reduces allocation when the string has a prefix that does not need to be encoded (1-2x speedup) and no regression when using a separator that's not '/' or the first char needs encoding. Interpreted performance is not affected much either: small positive when no encoding is needed, neutral or negligible regression otherwise. Thanks! /Claes
