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

[Volker Simonis]

Hi Chris,

this change is fine for me now and I think Arno addressed all of your
concerns. Is it OK for you if I push this to 9 and add you as one of
the reviewers?

Thank you and best regards,
Volker


On Wed, Jan 11, 2017 at 3:21 PM, Zeller, Arno  wrote:
> Hi Chris,
>
> I have addressed your comments in a new webrev. Can you please have a look at 
> this one?
>
> http://cr.openjdk.java.net/~clanger/webrevs/8170868.3/
>
> I created src/java.base/share/native/libnet/proxy_util.c/h and these files 
> contain now the common used native parts.
> And I rewrote the code to return an array of Proxy objects from native code - 
> so I do no longer call back to Java to
> add an object to the list.
>
> Best regards,
> Arno
>
>>-Original Message-
>>From: Chris Hegarty [mailto:chris.hega...@oracle.com]
>>Sent: Dienstag, 3. Januar 2017 15:04
>>To: Zeller, Arno 
>>Cc: net-dev@openjdk.java.net
>>Subject: Re: RFR:8170868: DefaultProxySelector should use system defaults
>>on Windows, MacOS and Gnome
>>
>>Arno,
>>
>>> On 27 Dec 2016, at 11:44, Zeller, Arno  wrote:
>>>
>>> Hi Chris,
>>>
>>> Thanks for having a look at my change:
>>>
 1) It seems awful to have to deal with LinkedList in native code. How
   about returning an array from native, and then converting that into
   whatever list type is appropriate at the Java level.
>>> With the current implementation on Mac I do not know upfront how many
>>items the list will contain and therefore I preferred to use the LinkedList 
>>from
>>native code.
>>> I can of course change the implementation on Mac to first generate a fully
>>resolved native list and then I can use NewObjectArray to generate an Array
>>of Proxy objects to return. This will avoid calling to java to add an element 
>>to
>>the list. Do you think this will be better?
>>
>>Yes. Thanks.
>>
 2) I would prefer the use of List.of(...), and list.of() for empty, since
   these are immutable and efficient list implementations.
>>> I thought about that but I tried to not return an immutable List because the
>>Javadoc of Proxy.select does not state anything about the returned List (if 
>>it is
>>immutable or not) and I feared to break compatibility by returning an
>>immutable List.
>>> If you think this is no problem I will of course prefer to always return the
>>same immutable NO_PROXY list object.
>>
>>I would prefer an immutable list. Maybe we file, a separate, issue to amend
>>the spec to say "returns an immutable list …”.
>>
 3) Is the comment “Inspired ...” necessary / appropriate?
>>> I will change it to the comment proposed by Volker.
>>
>>Ok.
>>
 4) Can some of the native initialization code be moved to a platform
   independent location, to remove duplication?
>>> Would it be ok if I move the definition of the static variables and the
>>implementation of 'static int initJavaClass(...)' to a header file in the 
>>shared
>>branch. I.e. src/java.base/shared/native/libnet/DefaultProxySelector.h and
>>include this in the MacOSX, Windows and the Unix implementations?
>>
>>Yes.
>>
 5) The new file has a shared copyright header. I see similar SAP
   headers in a few files, but none shared with the Oracle header.
   How did you arrive at this format?
>>> The format was suggested to me by Volker :-)
>>
>>Ok.
>>
>>-Chris.

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

[Chris Hegarty]

Arno,

> On 16 Jan 2017, at 11:23, Zeller, Arno  wrote:
> 
> Hi Michael and Chris,
> 
> I see that in with "RFR: 8172253 SetIfModifiedSince.java test fails with http 
> return code 404" you fix a problem 
> on macOS that does occur, because macOS is the only platform that sets a 
> default proxy from the system
> settings. On all other platforms the system settings are only used if 
> -Djava.net.useSystemProxies is set to true.
> 
> Do you know if there is a reason behind this behaviour?

The system proxies are used, by default, on Mac OS X to maintain compatibility
from Apple's Java 6 to OpenJDK 7 with Mac support ( 7 is the first OpenJDK
version that supports Mac ). 

> I would suggest that all platforms behave similar?

Maybe early on in a future release we could attempt to do this.

> A 
> reason could be that Mac users are often desktop users that are happy to have 
> the VM use their system 
> settings by default. But I guess this will apply for Windows  desktop Users 
> too and even for Linux or Solaris 
> users.
> 
> My webrev keeps the old behavior on macOS but I can change it :-)

Please keep the current behaviour, as you are proposing.

> The current behavior in the webrev is:
> 
> Default - without any properties:
> - macOS: If a proxy was manually added in the system settings it is used to 
> populate the http.proxyHost/Port,... 
>   properties (will not work with an auto config URL aka PAC file or WPAD 
> settings).
> - all other platforms: no proxy.
> 
> java.net.useSystemProxies=true:
> - macOS and Windows - use System functionality to get a proxy lists for a 
> given URL. This works now with PAC 
>   files or WPAD settings.
> - All UNIX versions excluding macOS: if Gnome 2.0 is configured to use a 
> proxy we use the system functionality
>  (available since gio lib 2.26) or we read the settings from gconf and use 
> them to populate the
>   http.proxyHost/Port,... properties - the later will again not work with PAC 
> files or WPAD.
> 
> http.proxyHost/Port,... properties are set:
> - On all platforms these properties have highest priority and will disable 
> the use of system settings. 
> 
> Of course by changing the default behavior we would behave incompatible to 
> old releases. What do you think 
> shall be done? 

Not in 9, but maybe in a future release.

> Another thing to improve could be the usage of the environment variables on 
> UNIX system. Currently  they are 
> ignored, but It is pretty easy to parse http_proxy, https_proxy, ftp_proxy 
> and no_proxy environment variables
> to set the default proxy properties on UNIX. My experience is that these 
> variables are used more often than 
> the gnome settings.

My personal preference is to move more towards the system’s proxies, rather
than yet another way of setting proxies.

-Chris.

> Best regards,
> Arno
> 
>> -Original Message-
>> From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of
>> Zeller, Arno
>> Sent: Mittwoch, 11. Januar 2017 15:21
>> To: Chris Hegarty 
>> Cc: net-dev@openjdk.java.net
>> Subject: RE: RFR:8170868: DefaultProxySelector should use system defaults on
>> Windows, MacOS and Gnome
>> 
>> Hi Chris,
>> 
>> I have addressed your comments in a new webrev. Can you please have a
>> look at this one?
>> 
>> http://cr.openjdk.java.net/~clanger/webrevs/8170868.3/
>> 
>> I created src/java.base/share/native/libnet/proxy_util.c/h and these files
>> contain now the common used native parts.
>> And I rewrote the code to return an array of Proxy objects from native code -
>> so I do no longer call back to Java to add an object to the list.
>> 
>> Best regards,
>> Arno
>> 
>>> -Original Message-
>>> From: Chris Hegarty [mailto:chris.hega...@oracle.com]
>>> Sent: Dienstag, 3. Januar 2017 15:04
>>> To: Zeller, Arno 
>>> Cc: net-dev@openjdk.java.net
>>> Subject: Re: RFR:8170868: DefaultProxySelector should use system
>>> defaults on Windows, MacOS and Gnome
>>> 
>>> Arno,
>>> 
 On 27 Dec 2016, at 11:44, Zeller, Arno  wrote:
 
 Hi Chris,
 
 Thanks for having a look at my change:
 
> 1) It seems awful to have to deal with LinkedList in native code. How
>  about returning an array from native, and then converting that into
>  whatever list type is appropriate at the Java level.
 With the current implementation on Mac I do not know upfront how many
>>> items the list will contain and therefore I preferred to use the
>>> LinkedList from native code.
 I can of course change the implementation on Mac to first generate a
 fully
>>> resolved native list and then I can use NewObjectArray to generate an
>>> Array of Proxy objects to return. This will avoid calling to java to
>>> add an element to the list. Do you think this will be better?
>>> 
>>> Yes. Thanks.
>>> 
> 2) I would prefer the use of List.of(...), and list.of() for empty, since
>  these are immutable and efficient list implementations.
 I thought about that but I tried to not return 

RE: RFR(M): 8167457: Fixes for InetAddressImpl native coding on Windows

[Langer, Christoph]

Hi Chris, Michael,

as I consider this change final from my side and the jdk10 forest is open now, 
I guess it is a good point to come back to this.

I personally would prefer if I could still submit it to jdk9, given that it is 
automatically consolidated to jdk10 and the code base would remain the same. 
The change set is only a cleanup to the Inet*AddressImpl.c coding and should be 
quite manageable.

The same goes for this one: 
http://mail.openjdk.java.net/pipermail/net-dev/2016-December/010571.html

In any case, I'd like you to decide this now and give me a review in short 
term, that I can get it out of my head. :)

Thanks & Best regards
Christoph

From: Langer, Christoph
Sent: Mittwoch, 21. Dezember 2016 15:57
To: net-dev@openjdk.java.net
Subject: RFR(M): 8167457: Fixes for InetAddressImpl native coding on Windows

Hi again,

after pushing the change for 8171077 
(http://hg.openjdk.java.net/jdk9/dev/jdk/rev/24f8703890b2), I have updated my 
patch for 8167457 and I have also updated the bug description to reflect the 
reduced scope.

Bug: https://bugs.openjdk.java.net/browse/JDK-8167457
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8167457.0/

The change now merely is code cleanup which should not change the current 
behavior apart from removing the isspace(hostname[0]) check and the 
isDottedIPAddress check in Java_java_net_Inet4AddressImpl_lookupAllHostAddr. 
But testing lets me think it is not needed any longer for the supported Windows 
versions of JDK9.

Same as with 8167420, from my point of view this change is ready to push and it 
runs nightly in our builds/tests of OpenJDK on several platforms. Though I 
guess you want to postpone this to JDK10, I'd be glad if you could still 
consider it for JDK9. But I won't pressure otherwise ;-)

Thanks & Best regards
Christoph

Re: RFR [9] Warning notice for types in Incubator Modules

[Magnus Ihse Bursie]

On 2017-01-25 13:51, Chris Hegarty wrote:

On 24 Jan 2017, at 16:44, Erik Joelsson  wrote:

Hello,

Build changes look good except for one thing. In Javadoc.gmk, the dependency on 
$(BUILD_TOOLS_JDK) needs to be set for $$($1_INDEX_FILE) (on line 317), where 
the actual recipes are created. Setting it on the phony docs-javadoc target 
will not help incremental builds.

Updated in place
   http://cr.openjdk.java.net/~chegar/incubator_taglet/


It's not ideal that a top-level target has dependencies from the JDK 
repo, but since we have no or few pre-existing top-level tools, I 
understand if you hesitate to add a new framework for such tools. If we 
ever do introduce more such tools, I think we should move this along.


I think the new -taglet options to the DEFAULT_JAVADOC_OPTIONS variable. 
As  the documentation says, the DEFAULT_JAVADOC_TAGS is there to specify 
the ordering of tags in the output. Unless the -taglet options 
implicitly generates one or more -tag options in it's place, that's the 
wrong place to put it. If it does, perhaps a comment indicating this 
hidden behavior would be in place.


In the line: "$$($1_INDEX_FILE): $(BUILD_TOOLS_JDK) $$($1_VARDEPS_FILE) 
$$($1_PACKAGE_DEPS) $$($1_DEPS)", please use a double $ ($$) for all 
variables. While technically not necessary in this particular case, it's 
misleading and we've had our share of bugs due to single $ in eval'd macros.


In the line: "docs-javadoc: $(BUILD_TOOLS_JDK) $(TARGETS)", please 
remove the reference to BUILD_TOOLS_JDK. It is not needed.


/Magnus




-Chris.


/Erik


On 2017-01-24 15:08, Chris Hegarty wrote:

As per the guidance in JEP 11 [1], the javadoc for types in
incubator modules should have a clear and explicit warning
notice. To that end, I’ve added a simple inline taglet that can
be used to effectively inject a standard notice, and applied it
to all public types in the HTTP module ( I’ll cleanup the line
width formatting before pushing ).

http://cr.openjdk.java.net/~chegar/incubator_taglet/

-Chris.

P.S. this work is, somewhat, in preparation for when we move
to a single documentation bundle [2].

[1] http://openjdk.java.net/jeps/11
[2] http://openjdk.java.net/jeps/299

Re: RFR(M): 8167457: Fixes for InetAddressImpl native coding on Windows

[Michael McMahon]

Christoph,

I would prefer to not push these native code change to JDK 9 at this stage.
They look fine for JDK 10 though. Consider them both reviewed.

Thanks
Michael.

On 26/01/2017, 10:02, Langer, Christoph wrote:


Hi Chris, Michael,

as I consider this change final from my side and the jdk10 forest is 
open now, I guess it is a good point to come back to this.


I personally would prefer if I could still submit it to jdk9, given 
that it is automatically consolidated to jdk10 and the code base would 
remain the same. The change set is only a cleanup to the 
Inet*AddressImpl.c coding and should be quite manageable.


The same goes for this one: 
http://mail.openjdk.java.net/pipermail/net-dev/2016-December/010571.html


In any case, I'd like you to decide this now and give me a review in 
short term, that I can get it out of my head. J


Thanks & Best regards

Christoph

*From:*Langer, Christoph
*Sent:* Mittwoch, 21. Dezember 2016 15:57
*To:* net-dev@openjdk.java.net
*Subject:* RFR(M): 8167457: Fixes for InetAddressImpl native coding on 
Windows


Hi again,

after pushing the change for 8171077 
(http://hg.openjdk.java.net/jdk9/dev/jdk/rev/24f8703890b2), I have 
updated my patch for 8167457 and I have also updated the bug 
description to reflect the reduced scope.


Bug: https://bugs.openjdk.java.net/browse/JDK-8167457

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8167457.0/ 



The change now merely is code cleanup which should not change the 
current behavior apart from removing the isspace(hostname[0]) check 
and the isDottedIPAddress check in 
Java_java_net_Inet4AddressImpl_lookupAllHostAddr. But testing lets me 
think it is not needed any longer for the supported Windows versions 
of JDK9.


Same as with 8167420, from my point of view this change is ready to 
push and it runs nightly in our builds/tests of OpenJDK on several 
platforms. Though I guess you want to postpone this to JDK10, I'd be 
glad if you could still consider it for JDK9. But I won't pressure 
otherwise ;-)


Thanks & Best regards

Christoph

RE: RFR(M): 8167457: Fixes for InetAddressImpl native coding on Windows

[Langer, Christoph]

Hi Michael,

Thanks for reviewing. So I'll push them in jdk10.

Best regards
Christoph

From: Michael McMahon [mailto:michael.x.mcma...@oracle.com]
Sent: Donnerstag, 26. Januar 2017 15:51
To: Langer, Christoph 
Cc: Chris Hegarty ; net-dev@openjdk.java.net
Subject: Re: RFR(M): 8167457: Fixes for InetAddressImpl native coding on Windows

Christoph,

I would prefer to not push these native code change to JDK 9 at this stage.
They look fine for JDK 10 though. Consider them both reviewed.

Thanks
Michael.

On 26/01/2017, 10:02, Langer, Christoph wrote:
Hi Chris, Michael,

as I consider this change final from my side and the jdk10 forest is open now, 
I guess it is a good point to come back to this.

I personally would prefer if I could still submit it to jdk9, given that it is 
automatically consolidated to jdk10 and the code base would remain the same. 
The change set is only a cleanup to the Inet*AddressImpl.c coding and should be 
quite manageable.

The same goes for this one: 
http://mail.openjdk.java.net/pipermail/net-dev/2016-December/010571.html

In any case, I'd like you to decide this now and give me a review in short 
term, that I can get it out of my head. :)

Thanks & Best regards
Christoph

From: Langer, Christoph
Sent: Mittwoch, 21. Dezember 2016 15:57
To: net-dev@openjdk.java.net
Subject: RFR(M): 8167457: Fixes for InetAddressImpl native coding on Windows

Hi again,

after pushing the change for 8171077 
(http://hg.openjdk.java.net/jdk9/dev/jdk/rev/24f8703890b2), I have updated my 
patch for 8167457 and I have also updated the bug description to reflect the 
reduced scope.

Bug: https://bugs.openjdk.java.net/browse/JDK-8167457
Webrev: 
http://cr.openjdk.java.net/~clanger/webrevs/8167457.0/

The change now merely is code cleanup which should not change the current 
behavior apart from removing the isspace(hostname[0]) check and the 
isDottedIPAddress check in Java_java_net_Inet4AddressImpl_lookupAllHostAddr. 
But testing lets me think it is not needed any longer for the supported Windows 
versions of JDK9.

Same as with 8167420, from my point of view this change is ready to push and it 
runs nightly in our builds/tests of OpenJDK on several platforms. Though I 
guess you want to postpone this to JDK10, I'd be glad if you could still 
consider it for JDK9. But I won't pressure otherwise ;-)

Thanks & Best regards
Christoph