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

[Zeller, Arno]

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.hegarty at oracle.com]
>Sent: Dienstag, 31. Januar 2017 12:47
>To: Zeller, Arno <arno.zeller at sap.com>
>Cc: net-dev at 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.zeller at 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.hegarty at oracle.com]
>>> Sent: Montag, 30. Januar 2017 17:14
>>> To: Zeller, Arno <arno.zeller at sap.com>
>>> Cc: net-dev at 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.
>>>