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

classic Classic list List threaded Threaded
27 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Zeller, Arno
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 <[hidden email]> 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
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Langer, Christoph
Hi Arno,

this looks good. I had already reviewed your change.

Just as for one thing that Volker mentioned:

>- 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 also have the concern that if you return one static instance of the List, then someone could modify it. Alternatively one could preallocate a static list and return Collections.unmodifiableList() from it. But I don't know if that makes things faster or is even more overhead compared to always generating a new List.

Best regards
Christoph

> -----Original Message-----
> From: net-dev [mailto:[hidden email]] On Behalf Of Zeller,
> Arno
> Sent: Mittwoch, 21. Dezember 2016 13:53
> To: Volker Simonis <[hidden email]>
> Cc: [hidden email]
> Subject: RE: RFR:8170868: DefaultProxySelector should use system defaults on
> Windows, MacOS and Gnome
>
> 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 <[hidden email]> 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
> >>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Vyom Tewari
In reply to this post by Zeller, Arno
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 [hidden email] 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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Zeller, Arno

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:[hidden email]] On Behalf Of Vyom Tewari
Sent: Mittwoch, 21. Dezember 2016 17:08
To: [hidden email]
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 [hidden email] 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
 

 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Vyom Tewari



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 [[hidden email]] On Behalf Of Vyom Tewari
Sent: Mittwoch, 21. Dezember 2016 17:08
To: [hidden email]
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 [hidden email] 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
 

 


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Zeller, Arno

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:[hidden email]]
Sent: Donnerstag, 22. Dezember 2016 14:27
To: Zeller, Arno <[hidden email]>
Cc: [hidden email]
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 [[hidden email]] On Behalf Of Vyom Tewari
Sent: Mittwoch, 21. Dezember 2016 17:08
To: [hidden email]
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 [hidden email] 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
 

 

 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
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 <[hidden email]> 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:[hidden email]]
Sent: Donnerstag, 22. Dezember 2016 14:27
To: Zeller, Arno <[hidden email]>
Cc: [hidden email]


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 [[hidden email]] On Behalf Of Vyom Tewari
Sent: Mittwoch, 21. Dezember 2016 17:08
To: [hidden email]
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 [hidden email] 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
 

 

 


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Volker Simonis
On Thu, Dec 22, 2016 at 9:41 PM, Thomas Stüfe <[hidden email]> 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 <[hidden email]> 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:[hidden email]]
>> Sent: Donnerstag, 22. Dezember 2016 14:27
>> To: Zeller, Arno <[hidden email]>
>> Cc: [hidden email]
>>
>>
>> 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:[hidden email]] On Behalf Of Vyom
>> Tewari
>> Sent: Mittwoch, 21. Dezember 2016 17:08
>> To: [hidden email]
>> 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 <[hidden email]> 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
>>
>>
>>
>>
>>
>>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Chris Hegarty
In reply to this post by Zeller, Arno
Arno,

> On 22 Dec 2016, at 16:44, Zeller, Arno <[hidden email]> 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/

I welcome this change. It is long over due, so thanks for running with it.

I have yet to build and test the patch myself, but just a few initial comments:

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.

2) I would prefer the use of List.of(...), and list.of() for empty, since
    these are immutable and efficient list implementations.

3) Is the comment “Inspired ...” necessary / appropriate?

4) Can some of the native initialization code be moved to a platform
    independent location, to remove duplication?

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?

-Chris.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
In reply to this post by Volker Simonis


On Fri, Dec 23, 2016 at 11:25 AM, Volker Simonis <[hidden email]> wrote:
On Thu, Dec 22, 2016 at 9:41 PM, Thomas Stüfe <[hidden email]> 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.

Fine, but in that case there should not be a separate MacOS implementation either, because MacOs is Unix too. If the future plan is to lump all Unix implementations together in this one file, just excluding MacOS feels arbitrary.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Chris Hegarty

On 23 Dec 2016, at 15:06, Thomas Stüfe <[hidden email]> wrote:

On Fri, Dec 23, 2016 at 11:25 AM, Volker Simonis <[hidden email]> wrote:
On Thu, Dec 22, 2016 at 9:41 PM, Thomas Stüfe <[hidden email]> wrote:
> ...
> 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.

+1  The code here applies to both Solaris and Linux, but not
OS X. It’s a little unusual, but seems fine.

Fine, but in that case there should not be a separate MacOS implementation either, because MacOs is Unix too. If the future plan is to lump all Unix implementations together in this one file, just excluding MacOS feels arbitrary.

There is a directory, ‘unix’, for generic Unix-like OS’s, and also
an OS-specific directory for code that is specific for a particular
Unix variant. It’s a little unusual here that two OS’s, Solaris and
Linux, share a common implementation, for Gnome, while OS
X has its own implementation, but what Arno has done seems
reasonable.

-Chris.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2


On Fri, Dec 23, 2016 at 4:57 PM, Chris Hegarty <[hidden email]> wrote:

On 23 Dec 2016, at 15:06, Thomas Stüfe <[hidden email]> wrote:

On Fri, Dec 23, 2016 at 11:25 AM, Volker Simonis <[hidden email]> wrote:
On Thu, Dec 22, 2016 at 9:41 PM, Thomas Stüfe <[hidden email]> wrote:
> ...
> 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.

+1  The code here applies to both Solaris and Linux, but not
OS X. It’s a little unusual, but seems fine.

Fine, but in that case there should not be a separate MacOS implementation either, because MacOs is Unix too. If the future plan is to lump all Unix implementations together in this one file, just excluding MacOS feels arbitrary.

There is a directory, ‘unix’, for generic Unix-like OS’s, and also
an OS-specific directory for code that is specific for a particular
Unix variant. It’s a little unusual here that two OS’s, Solaris and
Linux, share a common implementation, for Gnome, while OS
X has its own implementation, but what Arno has done seems
reasonable.


Okay! If you all can live with it, I certainly can too :)

Merry Christmas to all of you.

..Thomas
 
-Chris.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Langer, Christoph
In reply to this post by Chris Hegarty
Hi Chris, Arno,

as for a few points:

> 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.
+1

> 2) I would prefer the use of List.of(...), and list.of() for empty, since
>     these are immutable and efficient list implementations.

This seems like a good API to use here. But it only exists in Java 9. So in case of downporting this to Java 8 (which we certainly want to do for our SAP JVM) we need an alternative, which could be the current way of doing it.

> 4) Can some of the native initialization code be moved to a platform
>     independent location, to remove duplication?
+1

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

This format is used at some places, for instance in files of the the os/aix port in hotspot. I just pick hotspot/src/os/aix/vm/attachListener_aix.cpp as the first one but there are more in this subdirectory. Is it appropriate? - I don't know...

Best regards
Christoph

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Volker Simonis
In reply to this post by Chris Hegarty
On Fri, Dec 23, 2016 at 2:57 PM, Chris Hegarty <[hidden email]> wrote:

> Arno,
>
>> On 22 Dec 2016, at 16:44, Zeller, Arno <[hidden email]> 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/
>
> I welcome this change. It is long over due, so thanks for running with it.
>
> I have yet to build and test the patch myself, but just a few initial comments:
>
> 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.
>
> 2) I would prefer the use of List.of(...), and list.of() for empty, since
>     these are immutable and efficient list implementations.
>
> 3) Is the comment “Inspired ...” necessary / appropriate?
>

I don't think it is necessary but in my opinion it is useful. I for
myself don't know much about OS X programming and a link to some
documentation seems helpful.

Is it appropriate? I'm not a lawyer :) but the Disclaimer in the
referenced material mentions that "Neither the name .. or logos of
Apple Inc. may be used to endorse or promote products derived from the
... Software". So maybe I'd rephrase it to:

/**
 * For more information on how to use the APIs in "CFProxySupport.h" see:
 * https://developer.apple.com/legacy/library/samplecode/CFProxySupportTool/Introduction/Intro.html
 */

> 4) Can some of the native initialization code be moved to a platform
>     independent location, to remove duplication?
>
> 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?

We've discussed that before and as Christoph already mentioned this
pattern is also used in other places. The rule of thumb is the
following:
 - if we create a new file file from scratch it only has a SAP copyright
 - if we copy a file from an existing file and just add some
fixes/minor stuff it keeps its original copyright
 - if we derive a new file from an existing one but add a significant
amount of new code we keep the original copyright line but also add a
new one from SAP

There's always room for interpretation when it comes to this
classification, but I think overall it is reasonable (and you can find
many examples in the code base if you grep for files with more than
one copyright line).


>
> -Chris.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Zeller, Arno
In reply to this post by Chris Hegarty
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?

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

>3) Is the comment “Inspired ...” necessary / appropriate?
I will change it to the comment proposed by Volker.

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


>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 :-)

Best regards,
Arno
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Chris Hegarty
Arno,

> On 27 Dec 2016, at 11:44, Zeller, Arno <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Zeller, Arno
Hi Chris,

I have addressed your comments in a new webrev. Can you please have a look at this one?

http://cr.openjdk.java.net/~clanger/webrevs/8170868.3/

I created src/java.base/share/native/libnet/proxy_util.c/h and these files contain now the common used native parts.
And I rewrote the code to return an array of Proxy objects from native code - so I do no longer call back to Java to
add an object to the list.

Best regards,
Arno

>-----Original Message-----
>From: Chris Hegarty [mailto:[hidden email]]
>Sent: Dienstag, 3. Januar 2017 15:04
>To: Zeller, Arno <[hidden email]>
>Cc: [hidden email]
>Subject: Re: RFR:8170868: DefaultProxySelector should use system defaults
>on Windows, MacOS and Gnome
>
>Arno,
>
>> On 27 Dec 2016, at 11:44, Zeller, Arno <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Zeller, Arno
Hi Michael and Chris,

I see that in with "RFR: 8172253 SetIfModifiedSince.java test fails with http return code 404" you fix a problem
on macOS that does occur, because macOS is the only platform that sets a default proxy from the system
settings. On all other platforms the system settings are only used if -Djava.net.useSystemProxies is set to true.

Do you know if there is a reason behind this behavior? I would suggest that all platforms behave similar? A
reason could be that Mac users are often desktop users that are happy to have the VM use their system
settings by default. But I guess this will apply for Windows  desktop Users too and even for Linux or Solaris
users.

My webrev keeps the old behavior on macOS but I can change it :-)

The current behavior in the webrev is:

Default - without any properties:
- macOS: If a proxy was manually added in the system settings it is used to populate the http.proxyHost/Port,...
   properties (will not work with an auto config URL aka PAC file or WPAD settings).
- all other platforms: no proxy.

java.net.useSystemProxies=true:
- macOS and Windows - use System functionality to get a proxy lists for a given URL. This works now with PAC
   files or WPAD settings.
- All UNIX versions excluding macOS: if Gnome 2.0 is configured to use a proxy we use the system functionality
  (available since gio lib 2.26) or we read the settings from gconf and use them to populate the
   http.proxyHost/Port,... properties - the later will again not work with PAC files or WPAD.

http.proxyHost/Port,... properties are set:
- On all platforms these properties have highest priority and will disable the use of system settings.

Of course by changing the default behavior we would behave incompatible to old releases. What do you think
shall be done?

Another thing to improve could be the usage of the environment variables on UNIX system. Currently  they are
ignored, but It is pretty easy to parse http_proxy, https_proxy, ftp_proxy and no_proxy environment variables
to set the default proxy properties on UNIX. My experience is that these variables are used more often than
the gnome settings.

Best regards,
Arno

>-----Original Message-----
>From: net-dev [mailto:[hidden email]] On Behalf Of
>Zeller, Arno
>Sent: Mittwoch, 11. Januar 2017 15:21
>To: Chris Hegarty <[hidden email]>
>Cc: [hidden email]
>Subject: RE: RFR:8170868: DefaultProxySelector should use system defaults on
>Windows, MacOS and Gnome
>
>Hi Chris,
>
>I have addressed your comments in a new webrev. Can you please have a
>look at this one?
>
>http://cr.openjdk.java.net/~clanger/webrevs/8170868.3/
>
>I created src/java.base/share/native/libnet/proxy_util.c/h and these files
>contain now the common used native parts.
>And I rewrote the code to return an array of Proxy objects from native code -
>so I do no longer call back to Java to add an object to the list.
>
>Best regards,
>Arno
>
>>-----Original Message-----
>>From: Chris Hegarty [mailto:[hidden email]]
>>Sent: Dienstag, 3. Januar 2017 15:04
>>To: Zeller, Arno <[hidden email]>
>>Cc: [hidden email]
>>Subject: Re: RFR:8170868: DefaultProxySelector should use system
>>defaults on Windows, MacOS and Gnome
>>
>>Arno,
>>
>>> On 27 Dec 2016, at 11:44, Zeller, Arno <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Volker Simonis
In reply to this post by Zeller, Arno
Hi Chris,

this change is fine for me now and I think Arno addressed all of your
concerns. Is it OK for you if I push this to 9 and add you as one of
the reviewers?

Thank you and best regards,
Volker


On Wed, Jan 11, 2017 at 3:21 PM, Zeller, Arno <[hidden email]> wrote:

> Hi Chris,
>
> I have addressed your comments in a new webrev. Can you please have a look at this one?
>
> http://cr.openjdk.java.net/~clanger/webrevs/8170868.3/
>
> I created src/java.base/share/native/libnet/proxy_util.c/h and these files contain now the common used native parts.
> And I rewrote the code to return an array of Proxy objects from native code - so I do no longer call back to Java to
> add an object to the list.
>
> Best regards,
> Arno
>
>>-----Original Message-----
>>From: Chris Hegarty [mailto:[hidden email]]
>>Sent: Dienstag, 3. Januar 2017 15:04
>>To: Zeller, Arno <[hidden email]>
>>Cc: [hidden email]
>>Subject: Re: RFR:8170868: DefaultProxySelector should use system defaults
>>on Windows, MacOS and Gnome
>>
>>Arno,
>>
>>> On 27 Dec 2016, at 11:44, Zeller, Arno <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Chris Hegarty
In reply to this post by Zeller, Arno
Arno,

> On 16 Jan 2017, at 11:23, Zeller, Arno <[hidden email]> wrote:
>
> Hi Michael and Chris,
>
> I see that in with "RFR: 8172253 SetIfModifiedSince.java test fails with http return code 404" you fix a problem
> on macOS that does occur, because macOS is the only platform that sets a default proxy from the system
> settings. On all other platforms the system settings are only used if -Djava.net.useSystemProxies is set to true.
>
> Do you know if there is a reason behind this behaviour?

The system proxies are used, by default, on Mac OS X to maintain compatibility
from Apple's Java 6 to OpenJDK 7 with Mac support ( 7 is the first OpenJDK
version that supports Mac ).

> I would suggest that all platforms behave similar?

Maybe early on in a future release we could attempt to do this.

> A
> reason could be that Mac users are often desktop users that are happy to have the VM use their system
> settings by default. But I guess this will apply for Windows  desktop Users too and even for Linux or Solaris
> users.
>
> My webrev keeps the old behavior on macOS but I can change it :-)

Please keep the current behaviour, as you are proposing.

> The current behavior in the webrev is:
>
> Default - without any properties:
> - macOS: If a proxy was manually added in the system settings it is used to populate the http.proxyHost/Port,...
>   properties (will not work with an auto config URL aka PAC file or WPAD settings).
> - all other platforms: no proxy.
>
> java.net.useSystemProxies=true:
> - macOS and Windows - use System functionality to get a proxy lists for a given URL. This works now with PAC
>   files or WPAD settings.
> - All UNIX versions excluding macOS: if Gnome 2.0 is configured to use a proxy we use the system functionality
>  (available since gio lib 2.26) or we read the settings from gconf and use them to populate the
>   http.proxyHost/Port,... properties - the later will again not work with PAC files or WPAD.
>
> http.proxyHost/Port,... properties are set:
> - On all platforms these properties have highest priority and will disable the use of system settings.
>
> Of course by changing the default behavior we would behave incompatible to old releases. What do you think
> shall be done?

Not in 9, but maybe in a future release.

> Another thing to improve could be the usage of the environment variables on UNIX system. Currently  they are
> ignored, but It is pretty easy to parse http_proxy, https_proxy, ftp_proxy and no_proxy environment variables
> to set the default proxy properties on UNIX. My experience is that these variables are used more often than
> the gnome settings.

My personal preference is to move more towards the system’s proxies, rather
than yet another way of setting proxies.

-Chris.

> Best regards,
> Arno
>
>> -----Original Message-----
>> From: net-dev [mailto:[hidden email]] On Behalf Of
>> Zeller, Arno
>> Sent: Mittwoch, 11. Januar 2017 15:21
>> To: Chris Hegarty <[hidden email]>
>> Cc: [hidden email]
>> Subject: RE: RFR:8170868: DefaultProxySelector should use system defaults on
>> Windows, MacOS and Gnome
>>
>> Hi Chris,
>>
>> I have addressed your comments in a new webrev. Can you please have a
>> look at this one?
>>
>> http://cr.openjdk.java.net/~clanger/webrevs/8170868.3/
>>
>> I created src/java.base/share/native/libnet/proxy_util.c/h and these files
>> contain now the common used native parts.
>> And I rewrote the code to return an array of Proxy objects from native code -
>> so I do no longer call back to Java to add an object to the list.
>>
>> Best regards,
>> Arno
>>
>>> -----Original Message-----
>>> From: Chris Hegarty [mailto:[hidden email]]
>>> Sent: Dienstag, 3. Januar 2017 15:04
>>> To: Zeller, Arno <[hidden email]>
>>> Cc: [hidden email]
>>> Subject: Re: RFR:8170868: DefaultProxySelector should use system
>>> defaults on Windows, MacOS and Gnome
>>>
>>> Arno,
>>>
>>>> On 27 Dec 2016, at 11:44, Zeller, Arno <[hidden email]> 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.

12
Loading...