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: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/
<http://cr.openjdk.java.net/%7Eclanger/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/
<http://cr.openjdk.java.net/%7Eclanger/webrevs/8170868.0/>
Thanks a lot,
Arno Zeller
