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

[Erik Joelsson]

Hello,

In NetworkingLibraries.gmk, the explicit exclusion of 
$(JDK_TOPDIR)/src/java.base/unix/native/libnet/DefaultProxySelector.c 
shouldn't be necessary. SetupNativeCompilation should pick the first 
found source file of the same name (or rather, that would end up as the 
same object) and the macro FindSrcDirsForLib should guarantee that the 
most specific file is found first. If this doesn't work as intended it's 
a bug, but please try it. (If this doesn't work and you need to keep the 
exclusion, please fix indentation to 2 spaces for logical indent). Other 
than that it looks ok.

/Erik


On 2017-02-01 13:00, Chris Hegarty wrote:
Can someone from the build group please give the build part of
this the once over.

Thanks,
-Chris.

On 31 Jan 2017, at 19:14, Zeller, Arno <arno.zel...@sap.com> wrote:

Hi Chris,

thanks for all the improvements. I imported your webrev and prepared another 
webrev:
http://cr.openjdk.java.net/~clanger/webrevs/8170868.6/

I have seen the multiple DIRECTs myself during testing and I think filtering on 
the java side is a very elegant solution.
Thanks for this!

Best regards,
Arno

-----Original Message-----
From: Chris Hegarty [mailto:chris.hega...@oracle.com]
Sent: Dienstag, 31. Januar 2017 12:47
To: Zeller, Arno <arno.zel...@sap.com>
Cc: net-...@openjdk.java.net
Subject: Re: RFR:8170868: DefaultProxySelector should use system defaults
on Windows, MacOS and Gnome

Arno,

Thanks for doing this, and your patience so far. One more final round of
comments from me. Mostly minor. I’ve put them in the form of a webrev so
you can easily view and import them, if you agree.

http://cr.openjdk.java.net/~chegar/8170868_additional/

1)  A few comment rewordings and formatting suggestions.

2) I see duplicate DIRECT, and a few specific proxies, in my
   testing. Maybe my environment or PAC file is returning
   duplicate entries. I suggest that, at the java-level, we filter
   out duplicates while preserving ordering. ( see above webrev )

3) We do not use asserts in core libraries. So I replaced the usage
   with an early return NULL ( no proxy ). At a later stage we could
   throw an exception or something, but probably not all that
   interesting.

I have run this change ( along with my additional suggestions ) through our
internal build and test system. No surprises.

-Chris.

On 31 Jan 2017, at 09:14, Zeller, Arno <arno.zel...@sap.com> wrote:

Hi Chris,

thanks a lot - I did not see this case in my tests. I added your change to my
new webrev:
http://cr.openjdk.java.net/~clanger/webrevs/8170868.5/


Best regards,
Arno

-----Original Message-----
From: Chris Hegarty [mailto:chris.hega...@oracle.com]
Sent: Montag, 30. Januar 2017 17:14
To: Zeller, Arno <arno.zel...@sap.com>
Cc: net-...@openjdk.java.net
Subject: Re: RFR:8170868: DefaultProxySelector should use system
defaults on Windows, MacOS and Gnome

Arno,

I found an issue on Windows when proxies are specified per-protocol, i.e.
they are returned with their optional scheme set. I believe that the
scheme should be checked, at least without this change FTP proxies
were being returned for HTTP URL's, on my machine.

$ hg -R jdk diff
diff -r 07e07fecf383
src/java.base/windows/native/libnet/DefaultProxySelector.c
--- a/src/java.base/windows/native/libnet/DefaultProxySelector.c
Mon Jan 30 14:09:14 2017 +0000
+++ b/src/java.base/windows/native/libnet/DefaultProxySelector.c
Mon Jan 30 16:09:23 2017 +0000
@@ -99,6 +99,7 @@
* Returns the size of the array as int.
*/
static int createProxyList(LPWSTR win_proxy, const WCHAR *pproto,
list_item **head) {
+    static const wchar_t separators[] = L"\t\r\n ;";
    list_item *current = NULL;
    int nr_elems = 0;
    wchar_t *context = NULL;
@@ -109,13 +110,26 @@
     * The proxy server list contains one or more of the following
strings separated by semicolons or whitespace.
     * ([<scheme>=][<scheme>"://"]<server>[":"<port>])
     */
-    current_proxy = wcstok_s(win_proxy, L"; ", &context);
-    while (current_proxy != NULL) {
+    current_proxy = wcstok_s(win_proxy, separators, &context);
+    while (current_proxy != NULL) {
        LPWSTR pport;
        LPWSTR phost;
        int portVal = 0;
        wchar_t *next_proxy = NULL;
        list_item *proxy = NULL;
+        wchar_t* pos = NULL;
+
+        /* Filter based on the scheme, if there is one */
+        pos = wcschr(current_proxy, L'=');
+        if (pos) {
+          *pos = L'\0';
+          if (wcscmp(current_proxy, pproto) != 0) {
+              current_proxy = wcstok_s(NULL, separators, &context);
+              continue;
+          }
+          current_proxy = pos + 1;
+        }

        /* Let's check for a scheme and ignore it. */
        if ((phost = wcsstr(current_proxy, L"://")) != NULL) { @@
-152,7 +166,7 @@
            }
        }
        /* goto next proxy if available... */
-        current_proxy = wcstok_s(NULL, L"; ", &context);
+        current_proxy = wcstok_s(NULL, separators, &context);
    }
    return nr_elems;
}
@@ -268,7 +282,6 @@
        if (win_proxy != NULL) {
            wchar_t *context = NULL;
            int defport = 0;
-            WCHAR pproto[MAX_STR_LEN];
            int nr_elems = 0;

            /**
@@ -290,10 +303,7 @@
                goto noproxy;
            }

-            /* Prepare protocol string to compare with. */
-            _snwprintf(pproto, sizeof(pproto), L"%s=", lpProto);
-
-            nr_elems = createProxyList(win_proxy, pproto, &head);
+            nr_elems = createProxyList(win_proxy, lpProto, &head);
            if (nr_elems != 0 && head != NULL) {
                int index = 0;
                proxy_array = (*env)->NewObjectArray(env, nr_elems,
proxy_class, NULL);

-Chris.

P.S. I will take a look at the latest webrev.