Hi Vyom,
thanks you for having a look at my patch!
Regarding your suggestion to call CHECK_NULL_RETURN in DefaultProxySelector.c:
I guess you mean the Unix variant of DefaultProxySelector.c
(src/java.base/unix/native/libnet/DefaultProxySelector.c).
419 if (proxies) {
420 if (!error) {
421 int i;
422 // create an empty LinkedList to add the Proxy objects.
423 proxyList = (*env)->NewObject(env, list_class, list_ctrID);
424 if (proxyList != NULL) {
425 for(i = 0; proxies[i]; i++) {
...
454 }
455 }
456 }
457 (*g_strfreev)(proxies);
458 }
There I check in the next line if proxyList is NULL and skip the rest in this
case, but I cannot return without freeing the memory I got from the gnome
function call by calling (g_strfreev) later - otherwise there would be a memory
leak.
And in the Windows case it is the same pattern - at the point where I removed
the CHECK_NULL_RETURN macros I hold Windows system memory (in proxy_info and
ie_proxy_config) that I have to free with GlobalFree(...) and I also have to
release the JNI memory I hold with ReleaseStringChars(...).
I hope this explains the motivation of my change at this point.
Best regards,
Arno
From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of Vyom Tewari
Sent: Mittwoch, 21. Dezember 2016 17:08
To: net-dev@openjdk.java.net
Subject: Re: RFR:8170868: DefaultProxySelector should use system defaults on
Windows, MacOS and Gnome
Hi Arno,
I suggest you to call "CHECK_NULL_RETURN" after below line in
DefaultProxySelector.c
// create an empty LinkedList to add the Proxy objects.
+ proxyList = (*env)->NewObject(env, list_class, list_ctrID);
in windows/native/libnet/DefaultProxySelector.c file you remove the couple of
"CHECK_NULL_RETURN"
jstring jhost;
- if (pport == 0)
- pport = defport;
- jhost = (*env)->NewStringUTF(env, phost);
- CHECK_NULL_RETURN(jhost, NULL);
- isa = (*env)->CallStaticObjectMethod(env, isaddr_class,
isaddr_createUnresolvedID, jhost, pport);
- CHECK_NULL_RETURN(isa, NULL);
- proxy = (*env)->NewObject(env, proxy_class, proxy_ctrID, type_proxy,
isa);
- return proxy;
+ if (portVal == 0)
+ portVal = defport;
+ jhost = (*env)->NewString(env, phost, (jsize)wcslen(phost));
+ if (jhost != NULL) {
+ isa = (*env)->CallStaticObjectMethod(env, isaddr_class,
isaddr_createUnresolvedID, jhost, portVal);
+ if (isa != NULL) {
+ jobject proxy = (*env)->NewObject(env, proxy_class, proxy_ctrID,
type_proxy, isa);
+ if (proxy != NULL) {
+ (*env)->CallBooleanMethod(env, proxy_list, list_addID, proxy);
+ }
+ }
+ }
}
Is there is specific reason behind removing these checks ?
Thanks,
Vyom
On 12/21/2016 6:23 PM, Zeller, Arno wrote:
Hi Volker,
thanks for your valuable comments! I have a new patch ready that should address
your issues and contains also a forgotten change to the map file...
New webrev: http://cr.openjdk.java.net/~clanger/webrevs/8170868.1/
- make/lib/NetworkingLibraries.gmk
...
Have you tried to use
LIBNET_EXCLUDE_FILES :=
$(JDK_TOPDIR)/src/java.base/unix/native/libnet/DefaultProxySelector.c
I think this should work and it would mke it possible to rename:
src/java.base/macosx/native/libnet/DefaultProxySelector_mac.c
to:
src/java.base/macosx/native/libnet/DefaultProxySelector.c
which is much nicer IMHO :)
Great idea - it works and is of course the much nicer solution!
- DefaultProxySelector.java
322 return proxyList == null ? Arrays.asList(Proxy.NO_PROXY) :
proxyList;
Not sure if it would make sense to preallocate a static List with a single
Proxy.NO_PROXY element and always return that if proxyList equals null?
I return a new List object each time, because the select(URI uri) method does
not state anything about
not modifying the returned list. In case I return a static list containing only
the NO_PROXY element
a caller could remove the object from the list and other caller would use the
same modified list.
To avoid this I always create a new List object.
- java.base/unix/native/libnet/DefaultProxySelector.c
You've removed "#include <strings.h>". Have you built on all Unix platforms
(AIX, Solaris) to make sure you don't break anything?
It compiled on Linux, AIX, Solaris and Mac without problems for me.
- java.base/windows/native/libnet/DefaultProxySelector.c
Not sure if I understand this right, but in the gconf case above you insert all
proxies returned by "g_proxy_resolver_lookup" into the prox-list returned by
DefaultProxySelector_getSystemProxies. In the Windows case you write:
247 * From MSDN: The proxy server list contains one or more of
the following strings separated by semicolons or whitespace.
248 * ([<scheme>=][<scheme>"://"]<server>[":"<port>])
249 * We will only take the first entry here because the
java.net.Proxy class has only one entry.
Why can't you build up a proxy list here in the same way you do it for the
gconf case on Unix?
Sorry - I just forgot to implement it. Good that you found it. The new webrev
contains the missing functionality.
- src/java.base/macosx/native/libnet/DefaultProxySelector_mac.c
76 #define kResolveProxyRunLoopMode
CFSTR("com.sap.jvm.DefaultProxySelector")
I'm not familiar with the Mac programming model, but I don't think
"com.sap.jvm.DefaultProxySelector" is a good name for
kResolveProxyRunLoopMode. It should be something like
"java.net.DefaultProxySelector" but I'm open for better proposals :)
You are right - I changed it to "sun.net.spi.DefaultProxySelector".
PS: for your next RFR, can you please also add the estimated sisze and the bug
id to the subject line (e.g. "RFR(M):
8170868:DefaultProxySelector should..."). This makes it much easier to find a
review thread :)
I'll do my best next time...
Best regards,
Arno
On Wed, Dec 14, 2016 at 5:06 PM, Zeller, Arno
<arno.zel...@sap.com><mailto:arno.zel...@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
