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

[Vyom Tewari]

Hi Arno,

I suggest you to call "CHECK_NULL_RETURN" after below line in DefaultProxySelector.c
// create an empty LinkedList to add the Proxy objects.
+ proxyList = (*env)->NewObject(env, list_class, list_ctrID); in windows/native/libnet/DefaultProxySelector.c file you remove the couple of "CHECK_NULL_RETURN"
  
  jstring jhost;
- if (pport == 0)
- pport = defport;
- jhost = (*env)->NewStringUTF(env, phost);
- CHECK_NULL_RETURN(jhost, NULL);
- isa = (*env)->CallStaticObjectMethod(env, isaddr_class, 
isaddr_createUnresolvedID, jhost, pport);
- CHECK_NULL_RETURN(isa, NULL);
- proxy = (*env)->NewObject(env, proxy_class, proxy_ctrID, type_proxy, isa);
- return proxy;
+ if (portVal == 0)
+ portVal = defport;
+ jhost = (*env)->NewString(env, phost, (jsize)wcslen(phost));
+ if (jhost != NULL) {
+ isa = (*env)->CallStaticObjectMethod(env, isaddr_class, 
isaddr_createUnresolvedID, jhost, portVal);
+ if (isa != NULL) {
+ jobject proxy = (*env)->NewObject(env, proxy_class, proxy_ctrID, 
type_proxy, isa);
+ if (proxy != NULL) {
+ (*env)->CallBooleanMethod(env, proxy_list, list_addID, proxy);
+ }
+ }
+ }
          }


Is there is specific reason behind removing these checks ?

Thanks,
Vyom


On 12/21/2016 6:23 PM, Zeller, Arno wrote:
> Hi Volker,
>
> thanks for your valuable comments! I have a new patch ready that should address your issues and contains also a forgotten change to the map file...
>
> New webrev: http://cr.openjdk.java.net/~clanger/webrevs/8170868.1/
>
>
>> - make/lib/NetworkingLibraries.gmk
> ...
>> Have you tried to use
>> LIBNET_EXCLUDE_FILES :=
>> $(JDK_TOPDIR)/src/java.base/unix/native/libnet/DefaultProxySelector.c
>>
>> I think this should work and it would mke it possible to rename:
>> src/java.base/macosx/native/libnet/DefaultProxySelector_mac.c
>> to:
>> src/java.base/macosx/native/libnet/DefaultProxySelector.c
>> which is much nicer IMHO :)
> Great idea - it works and is of course the much nicer solution!
>
>> - DefaultProxySelector.java
>>
>> 322         return proxyList == null ? Arrays.asList(Proxy.NO_PROXY) :
>> proxyList;
>>
>> Not sure if it would make sense to preallocate a static List with a single
>> Proxy.NO_PROXY element and always return that if proxyList equals null?
> I return a new List object each time, because the select(URI uri) method does not state anything about
> not modifying the returned list. In case I return a static list containing only the NO_PROXY element
> a caller could remove the object from the list and other caller would use the same modified list.
> To avoid this I always create a new List object.
>
>> - java.base/unix/native/libnet/DefaultProxySelector.c
>>
>> You've removed "#include <strings.h>". Have you built on all Unix platforms
>> (AIX, Solaris) to make sure you don't break anything?
> It compiled on Linux, AIX, Solaris and Mac without problems for me.
>
>> - java.base/windows/native/libnet/DefaultProxySelector.c
>>
>> Not sure if I understand this right, but in the gconf case above you insert all
>> proxies returned by "g_proxy_resolver_lookup" into the prox-list returned by
>> DefaultProxySelector_getSystemProxies. In the Windows case you write:
>>
>> 247        * From MSDN: The proxy server list contains one or more of
>> the following strings separated by semicolons or whitespace.
>> 248        * ([<scheme>=][<scheme>"://"]<server>[":"<port>])
>> 249        * We will only take the first entry here because the
>> java.net.Proxy class has only one entry.
>>
>> Why can't you build up a proxy list here in the same way you do it for the
>> gconf case on Unix?
> Sorry - I just forgot to implement it. Good that you found it. The new webrev contains the missing functionality.
>
>> - src/java.base/macosx/native/libnet/DefaultProxySelector_mac.c
>>
>>   76 #define kResolveProxyRunLoopMode
>> CFSTR("com.sap.jvm.DefaultProxySelector")
>>
>>
>> I'm not familiar with the Mac programming model, but I don't think
>> "com.sap.jvm.DefaultProxySelector" is a good name for
>> kResolveProxyRunLoopMode. It should be something like
>> "java.net.DefaultProxySelector" but I'm open for better proposals :)
> You are right - I changed it to "sun.net.spi.DefaultProxySelector".
>
>> PS: for your next RFR, can you please also add the estimated sisze and the bug
>> id to the subject line (e.g. "RFR(M):
>> 8170868:DefaultProxySelector should..."). This makes it much easier to find a
>> review thread :)
> I'll do my best next time...
>
> Best regards,
> Arno
>
>> On Wed, Dec 14, 2016 at 5:06 PM, Zeller, Arno <arno.zeller at sap.com> wrote:
>>> Hi,
>>>
>>> can you please review my proposal for bug 8170868 - DefaultProxySelector
>> should use system defaults on Windows, MacOS and Gnome.
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8170868
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~clanger/webrevs/8170868.0/
>>>
>>> Thanks a lot,
>>> Arno Zeller
>>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20161221/23aeb27e/attachment.html>