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

Wed Feb 1 12:35:13 UTC 2017

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