RFR:8170868: DefaultProxySelector should use system defaults on Windows, MacOS and Gnome

Thu Dec 22 10:38:56 UTC 2016

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.

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.

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/

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

Thanks a lot,

Arno Zeller

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20161222/fdbc18a5/attachment-0001.html>