Re: [teststabilization] RFR: 8223573: Replace wildcard address with loopback or local host in tests - part 4

[Alan Bateman]

On 08/05/2019 18:24, Daniel Fuchs wrote:

Hi,

Please find below another patch in the series for intermittently
failing tests:

8223573: Replace wildcard address with loopback or local host in
 tests - part 4
https://bugs.openjdk.java.net/browse/JDK-8223573

patch:
http://cr.openjdk.java.net/~dfuchs/webrev_8223573/webrev.00/
I skimmed through the diffs and they look okay with me. Minor nit in 
6550798/test.java is there is a space in "HttpServer.create (". Also 
wonder if we should rename this source file too.


-Alan

Re: [teststabilization] RFR: 8223573: Replace wildcard address with loopback or local host in tests - part 4

[Daniel Fuchs]

On 09/05/2019 08:09, Alan Bateman wrote:

http://cr.openjdk.java.net/~dfuchs/webrev_8223573/webrev.00/
I skimmed through the diffs and they look okay with me. Minor nit in 
6550798/test.java is there is a space in "HttpServer.create (". Also 
wonder if we should rename this source file too.


Thanks Alan!

I'll grant you that the name of that test is not ideal.
But since it's not a new test I think I'll prefer to keep its original
name. I have removed the offending spaces - at several places
in that test.

Meanwhile I also found two other tests that needed updating:
test/jdk/sun/net/www/http/ChunkedInputStream/ChunkedEncodingWithProgressMonitorTest.java
test/jdk/sun/net/www/http/ChunkedInputStream/TestAvailable.java

I've added them to the webrev:

http://cr.openjdk.java.net/~dfuchs/webrev_8223573/webrev.01/

best regards,

-- daniel

Re: [teststabilization] RFR: 8223573: Replace wildcard address with loopback or local host in tests - part 4

[Chris Hegarty]

Daniel,

> On 9 May 2019, at 10:21, Daniel Fuchs  wrote:
> 
> ...
> Meanwhile I also found two other tests that needed updating:
> test/jdk/sun/net/www/http/ChunkedInputStream/ChunkedEncodingWithProgressMonitorTest.java
> test/jdk/sun/net/www/http/ChunkedInputStream/TestAvailable.java
> 
> I've added them to the webrev:
> 
> http://cr.openjdk.java.net/~dfuchs/webrev_8223573/webrev.01/

Looks good.

I’m missing why ChunkedEncodingWithProgressMonitorTest.java
needs `@library /test/lib` added?

-Chris.

Re: [teststabilization] RFR: 8223573: Replace wildcard address with loopback or local host in tests - part 4

[Daniel Fuchs]

Hi Chris,

On 09/05/2019 10:33, Chris Hegarty wrote:

http://cr.openjdk.java.net/~dfuchs/webrev_8223573/webrev.01/

Looks good.

I’m missing why ChunkedEncodingWithProgressMonitorTest.java
needs `@library /test/lib` added?


It depends on ChunkedInputStream/ChunkedEncodingTest.java, creates
an instance of that class.

I missed that in the previous webrev so I checked out all the
tests in that same directory.

best regards,

-- daniel

Re: [teststabilization] RFR: 8223573: Replace wildcard address with loopback or local host in tests - part 4

[Alan Bateman]

On 09/05/2019 10:21, Daniel Fuchs wrote:

On 09/05/2019 08:09, Alan Bateman wrote:

http://cr.openjdk.java.net/~dfuchs/webrev_8223573/webrev.00/
I skimmed through the diffs and they look okay with me. Minor nit in 
6550798/test.java is there is a space in "HttpServer.create (". Also 
wonder if we should rename this source file too.


Thanks Alan!

I'll grant you that the name of that test is not ideal.
But since it's not a new test I think I'll prefer to keep its original
name. I have removed the offending spaces - at several places
in that test.
Thanks, I see there is no more at L48 (you've removed one of them but 
there is a second).


In any case, the updated webrev looks good.

-Alan.

Re: [teststabilization] RFR: 8223573: Replace wildcard address with loopback or local host in tests - part 4

[Daniel Fuchs]

On 09/05/2019 10:52, Alan Bateman wrote:
Thanks, I see there is no more at L48 (you've removed one of them but 
there is a second).


In any case, the updated webrev looks good.


Argh! Thanks Alan - well spotted. I'll remove the offending
space before pushing.

best regards,

-- daniel

Re: 8218559: Reimplement the Legacy Socket API

[Michael McMahon]

On 07/05/2019, 17:14, Alan Bateman wrote:

On 07/05/2019 16:44, Michael McMahon wrote:

Hi Alan,

What's the purpose of the change to the UdpTest?
The deprecated Socket constructors can be used to create a Socket that 
uses UDP. This means the new SocketImpl has to support the creation of 
UDP sockets and use the ResourceManager to enforce limits when running 
with a  security manager.  So it's just increasing test coverage as I 
wasn't able to find any tests in test/jdk that exercise it.


-Alan
Ok, thanks. A test that might be useful could be one that compares the 
supported options between the old
and new impls for both client and server sockets. I guess it shouldn't 
be required for the initial push though.


- Michael.

Re: [ipv6] Regarding 8220673: Add test library support for determining platform IP support

[Chris Hegarty]

Alan,

> On 9 May 2019, at 07:22, Alan Bateman  wrote:
> 
> On 08/05/2019 12:02, Chris Hegarty wrote:
>> :
>>> 
>>> New webrev: http://cr.openjdk.java.net/~aeubanks/8220673/webrev.03/
>> 
>> :
>> 
>> P.S. adding nio-dev since there are a few tests in that area
>> being updated.
> Thanks for forwarding.
> 
> IPSupports.skipIfConcurrentConfigurationIsInvalid is a bit strange and I 
> think the first use of SkippedException in the jdk tests. I'm tempted to 
> suggest this should be a key that tests should select on via the @requires 
> tag but that may not be feasible because it depends on properties such as 
> preferIPv4Stack.

The alternative of using `@requires` was considered and rejected, for
the same reason you mention [1]. Additionally, using SkippedException
will be more straight forward to detect skipped tests and the reason
( the IP configuration ) why the test was skipped.

> So I guess the approach is okay but we should rename the method to start with 
> "skipTest" as it's not clear from the use-case what is being skipped. Also 
> "ConcurrentConfigurationIsInvalid" is vague and I think it would be useful if 
> the method description could emulate the conditions a bit better so that 
> someone seeing a IPSupport.skipTestXXX knows what is means.

I agree that the method name could be improved. It’s really:

“skip test if cannot create a minimally-operational IPv4 or IPv6 socket"

Some concrete method name suggestions ( more welcome ):

  - IPSupport::skipTestIfNonOperational
  - IPSupport::skipTestIfCannotCreateAnyIPSockets
  

Suggested method-level documentation improvement:

  /**
   * Ensures that the platform supports the ability to create a
   * minimally-operational socket whose protocol is either one of IPv4
   * or IPv6.
   *
   *  A minimally-operation socket is one that can be created and
   * bound to an IP-specific loopback address. IP support is
   * considered non-operational if a socket cannot be bound to either
   * one of, an IPv4 loopback address, or the IPv6 loopback address.
   *
   * @throws SkippedException if the current networking configuration
   * is non-operational
   */
  public static void skipTestIfNonOperational() throws SkippedException


-Chris.

P.S. 8220673 has been pushed already, but we should continue the
discussion here and I will create a follow-on JIRA issue to track it.

[1] https://mail.openjdk.java.net/pipermail/net-dev/2019-April/012348.html

Re: 8218559: Reimplement the Legacy Socket API

[Alan Bateman]

On 09/05/2019 11:51, Michael McMahon wrote:
Ok, thanks. A test that might be useful could be one that compares the 
supported options between the old
and new impls for both client and server sockets. I guess it shouldn't 
be required for the initial push though.


There are several existing tests that will fail if there options missing 
but they might not catch the scenario where the new implementation 
supports additional option. I can add another test for that as it's easy.


-Alan

Re: [ipv6] Regarding 8220673: Add test library support for determining platform IP support

[Alan Bateman]

On 09/05/2019 12:02, Chris Hegarty wrote:

:
I agree that the method name could be improved. It’s really:

“skip test if cannot create a minimally-operational IPv4 or IPv6 socket"

Some concrete method name suggestions ( more welcome ):

   - IPSupport::skipTestIfNonOperational
   - IPSupport::skipTestIfCannotCreateAnyIPSockets
   

throwSkippedExceptionIfXXX might work too. That might make it a bit more 
obvious in the tests.


-Alan

Re: [ipv6] RFR: 8223532: Don't try creating IPv4 sockets in NetworkInterface.c if IPv4 is not supported

[mark sheppard]

Hi Arthur et al.
with these changes and the increased separation of IPv4 and IPv6 in the 
call flows, does
it make sense to preclude the retrieval of IPv6 interfaces, if there is an 
error in the IPv4 processing in the
enumInterfaces function ?  Or  that an  error retrieving IPv6 interfaces 
precludes returning IPv4 interfaces already retrieved?

Is it worth considering  the alternative to record the error and continue the 
attempt to retrieve IPv6 interfaces.
If that succeeds clear the exception and return the IPv6 interfaces, and vice 
verse. An error in the IPv6 retrieval will
preclude the return of the IPv4 interfaces already retrieved.

A potential cause of failure in opening a socket is FD exhaustion at the moment 
of invocation (e.g. a constrained environment with open files 64 !!)
which may have dissipated, if the call flow proceeds to retrieve the IPv6, 
while at the same time another thread has released an fd with file close or 
socket close.

If not  then it may be worth  considering that any Exception thrown should 
elaborate the cause a bit more -- indicating the root cause is either IPv4 or 
IPv6 based,  as per openSocketWithFallback.

has the semantics of the openSocketWithFallback now changed ?
was it not that an AF_INET6 socket creation was attempted if the AF_INET (IPv4) 
socket creation failed ?
the predicate was on EPROTONOSUPPORT rather than EAFNOSUPPORT which is 
encaspulated in ipv4_available()
Now it's if ipv4 not available then try ipv6.
I know Chris made the point about treating them the same ... maybe so.
But at a pedantic level, the function is creating datagram socke heret, while 
IPv4_supported created a stream socket!!

anyway main point -- should a failure to retrieve IPv4 interfaces preclude IPv6 
interface retrieval and vice verse.


regards
Mark



From: net-dev  on behalf of Arthur Eubanks 

Sent: Wednesday 8 May 2019 17:33
To: Alan Bateman
Cc: OpenJDK Network Dev list
Subject: Re: [ipv6] RFR: 8223532: Don't try creating IPv4 sockets in 
NetworkInterface.c if IPv4 is not supported

Reverted changes in net_util.c.
Also, webrev.00 would create an IPv6 socket even if creating the IPv4 socket 
was successful. Fixed. (My very first revision had this same issue, which I 
thought I had fixed before sending it out. Tricky if statements have been 
cleaned up to make this less likely to happen again in the future.)

http://cr.openjdk.java.net/~aeubanks/8223532/webrev.01/

From: Alan Bateman mailto:alan.bate...@oracle.com>>
Date: Wed, May 8, 2019 at 7:58 AM
To: Chris Hegarty, Arthur Eubanks, OpenJDK Network Dev list



On 08/05/2019 12:51, Chris Hegarty wrote:
>
> While the vast majority of libnet.so is devoted to socket related
> implementation, not all is. There are a small number of low-level pieces
> of functionality that can be used with support for either IPv4 or IPv6
> being present. The NIO implementation also uses some shared common
> functionality from libnet.so. It seems overly restricting to disallow
> libnet.so from loading if neither IPv4 or IPv6 are present.
>
I agree as an innocent reference to a type in java.net might 
resulting
in libnet being loaded as a side effect. We also have the issue that the
file system implementation is in libnio so libnet will be loaded there
too (although shouldn't trigger its JNI OnLoad to be run).

-Alan

Re: 8218559: Reimplement the Legacy Socket API

[Alan Bateman]

Thanks Chris and Michael for the reviews/help so far. I've uploaded a 
new webrev to address most of their points here:

   http://cr.openjdk.java.net/~alanb/8221481/2/webrev/index.html

and delta webrev with the changes from 1 to 2 here:
   http://cr.openjdk.java.net/~alanb/8221481/1to2/webrev/index.html

-Alan

Re: 8218559: Reimplement the Legacy Socket API

[Chris Hegarty]

Alan,

> On 9 May 2019, at 14:00, Alan Bateman  wrote:
> 
> Thanks Chris and Michael for the reviews/help so far. I've uploaded a new 
> webrev to address most of their points here:
>http://cr.openjdk.java.net/~alanb/8221481/2/webrev/index.html
> 
> and delta webrev with the changes from 1 to 2 here:
>http://cr.openjdk.java.net/~alanb/8221481/1to2/webrev/index.html


This addresses my comments / concerns. Reviewed.

-Chris.

P.S. Thanks for generating the delta-webrev.

Re: 8218559: Reimplement the Legacy Socket API

[Michael McMahon]

Looks good to me. I don't have anything else to add at this point. I 
agree the new implementation is much clearer

than the old and it will be great to see it working in the field.

Thanks,

Michael

On 09/05/2019, 14:00, Alan Bateman wrote:
Thanks Chris and Michael for the reviews/help so far. I've uploaded a 
new webrev to address most of their points here:

   http://cr.openjdk.java.net/~alanb/8221481/2/webrev/index.html

and delta webrev with the changes from 1 to 2 here:
   http://cr.openjdk.java.net/~alanb/8221481/1to2/webrev/index.html

-Alan

Re: RFR: 8223553: Fix code constructs that do not compile with the Eclipse Java Compiler

[Daniel Fuchs]

Hi Christoph,

I'm only commenting on the HttpClient changes, I'll let
others comment on the other changes...

http://cr.openjdk.java.net/~clanger/webrevs/8223553.0/src/java.net.http/share/classes/jdk/internal/net/http/ExchangeImpl.java.udiff.html

I have a profound dislike for using @SuppressedWarnings, unless
it's the only alternative. My preference would be to introduce
local variables, if that can make the compiler happy, until such time
where the issue is actually resolved...

For instance, it seems that the following would work:

// Use local variable assignments to keep other java compilers
// happy and avoid unchecked casts warnings
BiFunctionExchangeImpl>>
factory = (h2c, t) -> createExchangeImpl(h2c, t, exchange, 
connection);
Function>, 
CompletableFuture>>

identity = (cf) -> cf;
return c2f.handle(factory).thenCompose(identity);


best regards,

-- daniel

On 08/05/2019 09:02, Langer, Christoph wrote:

Hi,

please review a small change that I’d like to see in OpenJDK to get rid 
of compilation errors in the Eclipse IDE.


It seems the root cause for the compilation errors is that javac would 
sometimes widen capture variables and is hence able to compile the 
places that I touch here. The EC4J compiler claims that it’s following 
the spec more strictly and bails out at these places. I had posted about 
this on compiler-dev but got no reaction [0].


So, as this seems to be some field of open work for the compiler/spec 
folks, I’d like to get EC4J quiesced by introducing some slight 
modifications to the original places which makes the code appeal a bit 
less elegant and fluent but will get rid of the compile errors.


Please review:

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

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

The modification to 
src/java.base/share/classes/java/util/concurrent/ConcurrentSkipListMap.java 
belongs to JSR-166, so I don’t know if it needs some special handling.


Thanks & Best regards

Christoph

[0] 
https://mail.openjdk.java.net/pipermail/compiler-dev/2019-March/013054.html

Re: [ipv6] RFR: 8223532: Don't try creating IPv4 sockets in NetworkInterface.c if IPv4 is not supported

[Chris Hegarty]

Arthur,

> On 8 May 2019, at 18:33, Arthur Eubanks  wrote:
> 
> Reverted changes in net_util.c.
> Also, webrev.00 would create an IPv6 socket even if creating the IPv4 socket 
> was successful. Fixed. (My very first revision had this same issue, which I 
> thought I had fixed before sending it out. Tricky if statements have been 
> cleaned up to make this less likely to happen again in the future.)
> 
> http://cr.openjdk.java.net/~aeubanks/8223532/webrev.01/

The following is Solaris specific code:

1630 sock = socket(AF_INET6, SOCK_DGRAM, 0)

It is missing a trailing semi-colon, so does not compile on Solaris.

Also the else block where this code exists should probably be:
 
   else if (ipv6_available()) {

   , since it creates an AF_NET6 socket.

I’ll run some tests.

-Chris.

Re: [RFR]: 8223652: Rename IPSupport.skipIfCurrentConfigurationIsInvalid()

[Daniel Fuchs]

Hi Arthur,

Looks good to me.

best regards,

-- daniel

On 09/05/2019 18:40, Arthur Eubanks wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8223652
Webrev: http://cr.openjdk.java.net/~aeubanks/8223652/webrev.00/index.html

As mentioned in 
https://markmail.org/search/?q=aeubanks%40google.com#query:aeubanks%40google.com+page:1+mid:rjthqgdeghv6vyck+state:results, 
IPSupport.skipIfCurrentConfigurationIsInvalid() is ambiguous (how is it 
skipping the test? what is a configuration?)


Something like IPSupport.throwSkippedExceptionIfNonOperational() is 
probably better.

[teststabilization] RFR: 8223465: Replace wildcard address with loopback or local host in tests - part 3

[Aleks Efimov]

Hi,

Please help to review another part of test fixes to address intermittent 
failures.


Webrev:
http://cr.openjdk.java.net/~aefimov/8223465/00/

JBS:
https://bugs.openjdk.java.net/browse/JDK-8223465

With Best Regards,
Aleksei