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