On Thu, Dec 22, 2016 at 9:41 PM, Thomas Stüfe <thomas.stu...@gmail.com> wrote: > Hi Arno, > > good job, this is a nice addition! > > Some remarks/questions (not a full review): > > 1) The naming of the unix...DefaultProxySelector.c is confusing. Could we > rename it to gnome/../DefaultProxySelector? >
I don't think that would be easy. "unix" is actually a OS-category which includes all *nix-like operatng systems and Gnome is not an OS category in my opinion. I also think that in the future the DefaultProxySelector.c may also support other Unix desktop environments like for example KDE so leaving it under unix is fine for me. > 2) DefaultProxySelector.java > > - style nit: "that list will most of the time" -> "that list will typically" > > - There are a number of places where select() will now return null (among > others getSystemProxies() return value is now returned without nullcheck), > that cannot be right, or? Contract in ProxySelector.java says nothing about > returning null. > > If returning null is ok now, comment at start of function should be updated. > > 3) unix/native/libnet/DefaultProxySelector.c > > - Java_sun_net_spi_DefaultProxySelector_getSystemProxies: > > Small style nit: could you please rename the return variable proxy to > proxyList? > > - getProxyByGProxyResolver: > > We stop processing the proxy list returned by gnome at the first //direct > entry. Are you sure this is okay? It seems reasonable, but the documentation > at https://developer.gnome.org/gio/stable/GProxyResolver.html does not state > anything about //direct entries being the last in the list. > > Kind Regards, Thomas > > On Thu, Dec 22, 2016 at 5:44 PM, Zeller, Arno <arno.zel...@sap.com> wrote: >> >> Hi Vyom, >> >> >> >> thanks for the comments – now I understand the problem. I reworked all >> three platforms to check for exceptions and NULL if needed. >> >> Regarding the JNIReleaseString calls: I seem to be on the save sidether. >> They are listed as some of the few calls that can be called with a pending >> exception as described in >> http://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#exception_handling >> >> >> >> Please see my new webrev: >> >> http://cr.openjdk.java.net/~clanger/webrevs/8170868.2/ >> >> >> >> Thanks and best regards, >> >> Arno >> >> >> >> From: Vyom Tewari [mailto:vyom.tew...@oracle.com] >> Sent: Donnerstag, 22. Dezember 2016 14:27 >> To: Zeller, Arno <arno.zel...@sap.com> >> Cc: net-dev@openjdk.java.net >> >> >> Subject: Re: RFR:8170868: DefaultProxySelector should use system defaults >> on Windows, MacOS and Gnome >> >> >> >> >> >> >> >> On 12/22/2016 4:08 PM, Zeller, Arno wrote: >> >> Hi Vyom, >> >> >> >> thanks you for having a look at my patch! >> >> Regarding your suggestion to call CHECK_NULL_RETURN in >> DefaultProxySelector.c: >> >> I guess you mean the Unix variant of DefaultProxySelector.c >> (src/java.base/unix/native/libnet/DefaultProxySelector.c). >> >> >> >> 419 if (proxies) { >> >> 420 if (!error) { >> >> 421 int i; >> >> 422 // create an empty LinkedList to add the Proxy objects. >> >> 423 proxyList = (*env)->NewObject(env, list_class, >> list_ctrID); >> >> 424 if (proxyList != NULL) { >> >> 425 for(i = 0; proxies[i]; i++) { >> >> ... >> >> 454 } >> >> 455 } >> >> 456 } >> >> 457 (*g_strfreev)(proxies); >> >> 458 } >> >> >> >> There I check in the next line if proxyList is NULL and skip the rest in >> this case, but I cannot return without freeing the memory I got from the >> gnome function call by calling (g_strfreev) later - otherwise there would be >> a memory leak. >> >> >> >> yes that true, but if NewObject failed at line 423 then there will be >> pending JNI exception in "getSystemProxy" method which calls >> "getProxyByGProxyResolver" method. I will suggest you to check for any JNI >> exception after calling the "getProxyByGProxyResolver". >> >> And in the Windows case it is the same pattern - at the point where I >> removed the CHECK_NULL_RETURN macros I hold Windows system memory (in >> proxy_info and ie_proxy_config) that I have to free with GlobalFree(...) and >> I also have to release the JNI memory I hold with ReleaseStringChars(...). >> >> >> >> I hope this explains the motivation of my change at this point. >> >> it make sense but I am not the JNI expert may be some one else can >> comments if it is safe to call the functions like "ReleaseStringChars" etc >> even if there is pending JNI exception before(if NewString fails at 266) >> function call. >> >> Thanks, >> Vyom >> >> >> >> Best regards, >> >> Arno >> >> >> >> >> >> From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of Vyom >> Tewari >> Sent: Mittwoch, 21. Dezember 2016 17:08 >> To: net-dev@openjdk.java.net >> Subject: Re: RFR:8170868: DefaultProxySelector should use system defaults >> on Windows, MacOS and Gnome >> >> >> >> Hi Arno, >> >> >> >> I suggest you to call "CHECK_NULL_RETURN" after below line in >> DefaultProxySelector.c >> >> // create an empty LinkedList to add the Proxy objects. >> >> + proxyList = (*env)->NewObject(env, list_class, list_ctrID); >> >> >> >> in windows/native/libnet/DefaultProxySelector.c file you remove the couple >> of "CHECK_NULL_RETURN" >> >> >> >> jstring jhost; >> >> - if (pport == 0) >> >> - pport = defport; >> >> - jhost = (*env)->NewStringUTF(env, phost); >> >> - CHECK_NULL_RETURN(jhost, NULL); >> >> - isa = (*env)->CallStaticObjectMethod(env, isaddr_class, >> isaddr_createUnresolvedID, jhost, pport); >> >> - CHECK_NULL_RETURN(isa, NULL); >> >> - proxy = (*env)->NewObject(env, proxy_class, proxy_ctrID, >> type_proxy, isa); >> >> - return proxy; >> >> + if (portVal == 0) >> >> + portVal = defport; >> >> + jhost = (*env)->NewString(env, phost, (jsize)wcslen(phost)); >> >> + if (jhost != NULL) { >> >> + isa = (*env)->CallStaticObjectMethod(env, isaddr_class, >> isaddr_createUnresolvedID, jhost, portVal); >> >> + if (isa != NULL) { >> >> + jobject proxy = (*env)->NewObject(env, proxy_class, >> proxy_ctrID, type_proxy, isa); >> >> + if (proxy != NULL) { >> >> + (*env)->CallBooleanMethod(env, proxy_list, list_addID, >> proxy); >> >> + } >> >> + } >> >> + } >> >> } >> >> >> >> >> >> Is there is specific reason behind removing these checks ? >> >> >> >> Thanks, >> >> Vyom >> >> >> >> On 12/21/2016 6:23 PM, Zeller, Arno wrote: >> >> Hi Volker, >> >> >> >> thanks for your valuable comments! I have a new patch ready that should >> address your issues and contains also a forgotten change to the map file... >> >> >> >> New webrev: http://cr.openjdk.java.net/~clanger/webrevs/8170868.1/ >> >> >> >> >> >> - make/lib/NetworkingLibraries.gmk >> >> ... >> >> Have you tried to use >> >> LIBNET_EXCLUDE_FILES := >> >> $(JDK_TOPDIR)/src/java.base/unix/native/libnet/DefaultProxySelector.c >> >> >> >> I think this should work and it would mke it possible to rename: >> >> src/java.base/macosx/native/libnet/DefaultProxySelector_mac.c >> >> to: >> >> src/java.base/macosx/native/libnet/DefaultProxySelector.c >> >> which is much nicer IMHO :) >> >> >> >> Great idea - it works and is of course the much nicer solution! >> >> >> >> - DefaultProxySelector.java >> >> >> >> 322 return proxyList == null ? Arrays.asList(Proxy.NO_PROXY) : >> >> proxyList; >> >> >> >> Not sure if it would make sense to preallocate a static List with a single >> >> Proxy.NO_PROXY element and always return that if proxyList equals null? >> >> >> >> I return a new List object each time, because the select(URI uri) method >> does not state anything about >> >> not modifying the returned list. In case I return a static list containing >> only the NO_PROXY element >> >> a caller could remove the object from the list and other caller would use >> the same modified list. >> >> To avoid this I always create a new List object. >> >> >> >> - java.base/unix/native/libnet/DefaultProxySelector.c >> >> >> >> You've removed "#include <strings.h>". Have you built on all Unix >> platforms >> >> (AIX, Solaris) to make sure you don't break anything? >> >> >> >> It compiled on Linux, AIX, Solaris and Mac without problems for me. >> >> >> >> - java.base/windows/native/libnet/DefaultProxySelector.c >> >> >> >> Not sure if I understand this right, but in the gconf case above you >> insert all >> >> proxies returned by "g_proxy_resolver_lookup" into the prox-list returned >> by >> >> DefaultProxySelector_getSystemProxies. In the Windows case you write: >> >> >> >> 247 * From MSDN: The proxy server list contains one or more of >> >> the following strings separated by semicolons or whitespace. >> >> 248 * ([<scheme>=][<scheme>"://"]<server>[":"<port>]) >> >> 249 * We will only take the first entry here because the >> >> java.net.Proxy class has only one entry. >> >> >> >> Why can't you build up a proxy list here in the same way you do it for the >> >> gconf case on Unix? >> >> >> >> Sorry - I just forgot to implement it. Good that you found it. The new >> webrev contains the missing functionality. >> >> >> >> - src/java.base/macosx/native/libnet/DefaultProxySelector_mac.c >> >> >> >> 76 #define kResolveProxyRunLoopMode >> >> CFSTR("com.sap.jvm.DefaultProxySelector") >> >> >> >> >> >> I'm not familiar with the Mac programming model, but I don't think >> >> "com.sap.jvm.DefaultProxySelector" is a good name for >> >> kResolveProxyRunLoopMode. It should be something like >> >> "java.net.DefaultProxySelector" but I'm open for better proposals :) >> >> >> >> You are right - I changed it to "sun.net.spi.DefaultProxySelector". >> >> >> >> PS: for your next RFR, can you please also add the estimated sisze and the >> bug >> >> id to the subject line (e.g. "RFR(M): >> >> 8170868:DefaultProxySelector should..."). This makes it much easier to >> find a >> >> review thread :) >> >> >> >> I'll do my best next time... >> >> >> >> Best regards, >> >> Arno >> >> >> >> On Wed, Dec 14, 2016 at 5:06 PM, Zeller, Arno <arno.zel...@sap.com> wrote: >> >> Hi, >> >> >> >> can you please review my proposal for bug 8170868 - DefaultProxySelector >> >> should use system defaults on Windows, MacOS and Gnome. >> >> >> >> Bug: >> >> https://bugs.openjdk.java.net/browse/JDK-8170868 >> >> >> >> Webrev: >> >> http://cr.openjdk.java.net/~clanger/webrevs/8170868.0/ >> >> >> >> Thanks a lot, >> >> Arno Zeller >> >> >> >> >> >> > >
