RFR:8170868: DefaultProxySelector should use system defaults on Windows, MacOS and Gnome
Vyom Tewari
vyom.tewari at oracle.com
Thu Dec 22 13:27:07 UTC 2016
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-bounces at openjdk.java.net] *On Behalf Of
> *Vyom Tewari
> *Sent:* Mittwoch, 21. Dezember 2016 17:08
> *To:* net-dev at 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/
> <http://cr.openjdk.java.net/%7Eclanger/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.zeller at sap.com> <mailto:arno.zeller at 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/
> <http://cr.openjdk.java.net/%7Eclanger/webrevs/8170868.0/>
>
> Thanks a lot,
>
> Arno Zeller
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20161222/d65ff318/attachment-0001.html>
More information about the net-dev
mailing list