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