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? 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 > <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> > <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 > > > > > > >
