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

[Chris Hegarty]

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.zeller at 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.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.
>>>> 
>