Re: RFR: 8255583: Investigate creating a test to trigger the condition in KeepAliveStreamCleaner [v3]

2020-12-09 Thread Chris Hegarty 
On Mon, 7 Dec 2020 17:11:28 GMT, Conor Cleary  wrote:

>> The KeepAliveStreamCleaner in sun.net.ww.http package had been previously 
>> seen to fail with an IllegalMonitorStateException. This failure was caused 
>> by the use of `wait()` in a non synchronized block. This failure was 
>> mitigated through use of `await()` instead as is shown below (code can be 
>> viewed on L119 in 
>> src/java.base/share/classes/sun/net/www/http/KeepAliveStreamCleaner.java).
>> 
>> ...
>> long timeout = TIMEOUT;
>> while ((kace = poll()) == null) {
>> waiter.await(timeout, TimeUnit.MILLISECONDS); 
>> ...
>> While the code throwing the exception was fixed, a regression test was not 
>> made. So this patch adds a simple white-box test which calls the 
>> KeepAliveStreamCleaner's run method. `waiter.wait()` should always fail if 
>> not used in a synchronized block or method, so the test verifies that it is 
>> not used.
>
> Conor Cleary has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8255583: Removed unnecessary bug id

Marked as reviewed by chegar (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/1659

Re: RFR: 8255583: Investigate creating a test to trigger the condition in KeepAliveStreamCleaner [v3]

2020-12-09 Thread Michael McMahon 
On Mon, 7 Dec 2020 17:11:28 GMT, Conor Cleary  wrote:

>> The KeepAliveStreamCleaner in sun.net.ww.http package had been previously 
>> seen to fail with an IllegalMonitorStateException. This failure was caused 
>> by the use of `wait()` in a non synchronized block. This failure was 
>> mitigated through use of `await()` instead as is shown below (code can be 
>> viewed on L119 in 
>> src/java.base/share/classes/sun/net/www/http/KeepAliveStreamCleaner.java).
>> 
>> ...
>> long timeout = TIMEOUT;
>> while ((kace = poll()) == null) {
>> waiter.await(timeout, TimeUnit.MILLISECONDS); 
>> ...
>> While the code throwing the exception was fixed, a regression test was not 
>> made. So this patch adds a simple white-box test which calls the 
>> KeepAliveStreamCleaner's run method. `waiter.wait()` should always fail if 
>> not used in a synchronized block or method, so the test verifies that it is 
>> not used.
>
> Conor Cleary has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8255583: Removed unnecessary bug id

LGTM

-

Marked as reviewed by michaelm (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1659

Re: RFR: 8254996: make jdk.net.UnixDomainPrincipal a record class [v2]

2020-12-09 Thread Patrick Concannon 
> Hi,
> 
> Could someone please review my code for JDK-8254996: 'make 
> jdk.net.UnixDomainPrincipal a record class'?
> 
> `jdk.net.UnixDomainPrincipal` is a simple immutable data class that requires 
> boilerplate methods for access. However, these methods and fields are 
> susceptible to trivial mistakes and add to the verbosity of the class. 
> This fix replaces the data class with a record class, a new type in JDK 16, 
> making the code more concise and easier to maintain. This code is binary 
> compatible with the original. For the generated API doc change, please refer 
> to the specdiff attached to the CSR.
> 
> CSR : https://bugs.openjdk.java.net/browse/JDK-8257823
> 
> Kind regards,
> Patrick

Patrick Concannon has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains two additional 
commits since the last revision:

 - Merge remote-tracking branch 'origin/master' into JDK-8254996
 - 8254996: make jdk.net.UnixDomainPrincipal a record class

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1668/files
  - new: https://git.openjdk.java.net/jdk/pull/1668/files/7dace76c..2adf1455

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1668&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1668&range=00-01

  Stats: 11962 lines in 365 files changed: 6562 ins; 3982 del; 1418 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1668.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1668/head:pull/1668

PR: https://git.openjdk.java.net/jdk/pull/1668

RFR: 8256459: java/net/httpclient/ManyRequests.java and java/net/httpclient/LineBodyHandlerTest.java fail infrequently with java.net.ConnectException: Connection timed out: no further information

2020-12-09 Thread Daniel Fuchs 
Hi, 

Please find here a changeset that fixes the infrequent (but annoying) test 
failures
caused by unexpected ConnectionException "Connection timed out: no further 
information"
which have been observed to occur on some platforms.

Tests are updated to allow the test server to handle requests concurrently.
PlainHttpConnection is updated to retry connection once if chan::finishConnect 
fails
early with ConnectionException and the connection timeout has not expired.

-

Commit messages:
 - 8256459: java/net/httpclient/ManyRequests.java and 
java/net/httpclient/LineBodyHandlerTest.java fail infrequently with 
java.net.ConnectException: Connection timed out: no further information

Changes: https://git.openjdk.java.net/jdk/pull/1716/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1716&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8256459
  Stats: 163 lines in 4 files changed: 121 ins; 8 del; 34 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1716.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1716/head:pull/1716

PR: https://git.openjdk.java.net/jdk/pull/1716

Re: RFR: 8254996: make jdk.net.UnixDomainPrincipal a record class [v3]

2020-12-09 Thread Patrick Concannon 
> Hi,
> 
> Could someone please review my code for JDK-8254996: 'make 
> jdk.net.UnixDomainPrincipal a record class'?
> 
> `jdk.net.UnixDomainPrincipal` is a simple immutable data class that requires 
> boilerplate methods for access. However, these methods and fields are 
> susceptible to trivial mistakes and add to the verbosity of the class. 
> This fix replaces the data class with a record class, a new type in JDK 16, 
> making the code more concise and easier to maintain. This code is binary 
> compatible with the original. For the generated API doc change, please refer 
> to the specdiff attached to the CSR.
> 
> CSR : https://bugs.openjdk.java.net/browse/JDK-8257823
> 
> Kind regards,
> Patrick

Patrick Concannon has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains three additional 
commits since the last revision:

 - Merge remote-tracking branch 'origin/master' into JDK-8254996
 - Merge remote-tracking branch 'origin/master' into JDK-8254996
 - 8254996: make jdk.net.UnixDomainPrincipal a record class

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1668/files
  - new: https://git.openjdk.java.net/jdk/pull/1668/files/2adf1455..b00569d7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1668&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1668&range=01-02

  Stats: 132 lines in 7 files changed: 125 ins; 1 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1668.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1668/head:pull/1668

PR: https://git.openjdk.java.net/jdk/pull/1668

Integrated: 8254996: make jdk.net.UnixDomainPrincipal a record class

2020-12-09 Thread Patrick Concannon 
On Mon, 7 Dec 2020 15:32:54 GMT, Patrick Concannon  
wrote:

> Hi,
> 
> Could someone please review my code for JDK-8254996: 'make 
> jdk.net.UnixDomainPrincipal a record class'?
> 
> `jdk.net.UnixDomainPrincipal` is a simple immutable data class that requires 
> boilerplate methods for access. However, these methods and fields are 
> susceptible to trivial mistakes and add to the verbosity of the class. 
> This fix replaces the data class with a record class, a new type in JDK 16, 
> making the code more concise and easier to maintain. This code is binary 
> compatible with the original. For the generated API doc change, please refer 
> to the specdiff attached to the CSR.
> 
> CSR : https://bugs.openjdk.java.net/browse/JDK-8257823
> 
> Kind regards,
> Patrick

This pull request has now been integrated.

Changeset: 6dd06add
Author:Patrick Concannon 
URL:   https://git.openjdk.java.net/jdk/commit/6dd06add
Stats: 52 lines in 1 file changed: 3 ins; 45 del; 4 mod

8254996: make jdk.net.UnixDomainPrincipal a record class

Reviewed-by: dfuchs, michaelm

-

PR: https://git.openjdk.java.net/jdk/pull/1668

Re: RFR: 8256459: java/net/httpclient/ManyRequests.java and java/net/httpclient/LineBodyHandlerTest.java fail infrequently with java.net.ConnectException: Connection timed out: no further information

2020-12-09 Thread Chris Hegarty 
On Wed, 9 Dec 2020 15:37:42 GMT, Daniel Fuchs  wrote:

> Hi, 
> 
> Please find here a changeset that fixes the infrequent (but annoying) test 
> failures
> caused by unexpected ConnectionException "Connection timed out: no further 
> information"
> which have been observed to occur on some platforms.
> 
> Tests are updated to allow the test server to handle requests concurrently.
> PlainHttpConnection is updated to retry connection once if 
> chan::finishConnect fails
> early with ConnectionException and the connection timeout has not expired.

src/java.net.http/share/classes/jdk/internal/net/http/PlainHttpConnection.java 
line 205:

> 203: }
> 204: return cf.handle((r,t) -> checkRetryConnect(r, t,exchange))
> 205: .thenCompose(Function.identity());

So this changes behaviour so that a single subsequent additional TCP connection 
may be tried for all cases where the connection fails AND there is remaining 
connection deadline. Ok.

-

PR: https://git.openjdk.java.net/jdk/pull/1716

Re: RFR: 8256459: java/net/httpclient/ManyRequests.java and java/net/httpclient/LineBodyHandlerTest.java fail infrequently with java.net.ConnectException: Connection timed out: no further information

2020-12-09 Thread Chris Hegarty 
On Wed, 9 Dec 2020 15:37:42 GMT, Daniel Fuchs  wrote:

> Hi, 
> 
> Please find here a changeset that fixes the infrequent (but annoying) test 
> failures
> caused by unexpected ConnectionException "Connection timed out: no further 
> information"
> which have been observed to occur on some platforms.
> 
> Tests are updated to allow the test server to handle requests concurrently.
> PlainHttpConnection is updated to retry connection once if 
> chan::finishConnect fails
> early with ConnectionException and the connection timeout has not expired.

Marked as reviewed by chegar (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/1716

Re: RFR: 8256459: java/net/httpclient/ManyRequests.java and java/net/httpclient/LineBodyHandlerTest.java fail infrequently with java.net.ConnectException: Connection timed out: no further information

2020-12-09 Thread Daniel Fuchs 
On Wed, 9 Dec 2020 17:23:19 GMT, Chris Hegarty  wrote:

>> Hi, 
>> 
>> Please find here a changeset that fixes the infrequent (but annoying) test 
>> failures
>> caused by unexpected ConnectionException "Connection timed out: no further 
>> information"
>> which have been observed to occur on some platforms.
>> 
>> Tests are updated to allow the test server to handle requests concurrently.
>> PlainHttpConnection is updated to retry connection once if 
>> chan::finishConnect fails
>> early with ConnectionException and the connection timeout has not expired.
>
> src/java.net.http/share/classes/jdk/internal/net/http/PlainHttpConnection.java
>  line 205:
> 
>> 203: }
>> 204: return cf.handle((r,t) -> checkRetryConnect(r, t,exchange))
>> 205: .thenCompose(Function.identity());
> 
> So this changes behaviour so that a single subsequent additional TCP 
> connection may be tried for all cases where the connection fails AND there is 
> remaining connection deadline. Ok.

Yes - I didn't want to depend on the exception message - and there's no way to 
distinguish with the exception type (it's raw `ConnectionException`). It will 
however happen *only* when the `ConnectionException` is thrown by 
`SocketChannel::finishConnect`.

-

PR: https://git.openjdk.java.net/jdk/pull/1716

Re: icmp and raw sockets on linux

2020-12-09 Thread Jamie Le Tual 
I'm wondering what the process is for the pull request I submitted, I guess
I need a reviewer or a sponsor?


On Tue, 1 Dec 2020 at 14:08, Jamie Le Tual  wrote:

> Hi Alan,
> I received an email this morning saying my OCA had been processed, but
> that it would perhaps take a couple of hours before a bot picks it up and I
> show up on the OCA signatories page (no idea where that page is though).
>
> I have also updated the patch in my pull request to cover ipv6 as well.
>
> Test cases are forthcoming.
>
>
> On Sun, 29 Nov 2020 at 11:35, Alan Bateman 
> wrote:
>
>> On 29/11/2020 14:54, Jamie Le Tual wrote:
>>
>> Although I've only just sent in the pdf form for an ora, I've already
>> submitted a pull request, https://github.com/openjdk/jdk/pull/1502
>> 
>> wherein an attempt is first made to use an IPPROTO_ICMP socket before
>> falling back on RAW_SOCK and finally tcp echo.
>>
>> I suppose now I have to wait until I am able to open an issue in the bug
>> tracker so I can associate the pull request to it.
>>
>> Does anyone know what a unit test for this might look like? To cover the
>> use cases the code has to be invoked by both a privileged and unprivileged
>> user, and I'm not sure how to go about setting up a unit test.
>>
>>
>> I've created JDK-8257235 [1] to track this. One thing to understand is
>> whether there is an equivalent for IPPROTO_ICMPV6. Sorry, I can't look at
>> the patch or comment in the PR until the bot confirms that you have signed
>> the OCA.
>>
>> Have you looked at the existing tests in test/jdk/java/net/InetAddress?
>> It will be awkward to verify as the behaviour before/after will not be
>> observable without looking at the network or system call trace.
>>
>> -Alan
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8257235
>>
>

Re: RFR: 8256459: java/net/httpclient/ManyRequests.java and java/net/httpclient/LineBodyHandlerTest.java fail infrequently with java.net.ConnectException: Connection timed out: no further information

2020-12-09 Thread Daniel Fuchs 
> Hi, 
> 
> Please find here a changeset that fixes the infrequent (but annoying) test 
> failures
> caused by unexpected ConnectionException "Connection timed out: no further 
> information"
> which have been observed to occur on some platforms.
> 
> Tests are updated to allow the test server to handle requests concurrently.
> PlainHttpConnection is updated to retry connection once if 
> chan::finishConnect fails
> early with ConnectionException and the connection timeout has not expired.

Daniel Fuchs has updated the pull request incrementally with one additional 
commit since the last revision:

  8256459: java/net/httpclient/ManyRequests.java and 
java/net/httpclient/LineBodyHandlerTest.java fail infrequently with 
java.net.ConnectException: Connection timed out: no further information
  
  Connect will not be retried if retry connect is disabled
  JBS issue label noreg-self removed and bug id added to tests

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1716/files
  - new: https://git.openjdk.java.net/jdk/pull/1716/files/617c1f5a..090b390f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1716&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1716&range=00-01

  Stats: 12 lines in 5 files changed: 3 ins; 0 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1716.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1716/head:pull/1716

PR: https://git.openjdk.java.net/jdk/pull/1716

Re: RFR: 8256459: java/net/httpclient/ManyRequests.java and java/net/httpclient/LineBodyHandlerTest.java fail infrequently with java.net.ConnectException: Connection timed out: no further information

2020-12-09 Thread Daniel Fuchs 
On Wed, 9 Dec 2020 17:23:19 GMT, Chris Hegarty  wrote:

>> Daniel Fuchs has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   8256459: java/net/httpclient/ManyRequests.java and 
>> java/net/httpclient/LineBodyHandlerTest.java fail infrequently with 
>> java.net.ConnectException: Connection timed out: no further information
>>   
>>   Connect will not be retried if retry connect is disabled
>>   JBS issue label noreg-self removed and bug id added to tests
>
> src/java.net.http/share/classes/jdk/internal/net/http/PlainHttpConnection.java
>  line 205:
> 
>> 203: }
>> 204: return cf.handle((r,t) -> checkRetryConnect(r, t,exchange))
>> 205: .thenCompose(Function.identity());
> 
> So this changes behaviour so that a single subsequent additional TCP 
> connection may be tried for all cases where the connection fails AND there is 
> remaining connection deadline. Ok.

@ChrisHegarty I have modified the logic to not retry if 
`jdk.httpclient.disableRetryConnect` was specified. By default that property is 
not specified and so we will retry once, but if an application has specifically 
opted for not retrying on ConnectException then we will not retry.

-

PR: https://git.openjdk.java.net/jdk/pull/1716

Re: RFR: 8256459: java/net/httpclient/ManyRequests.java and java/net/httpclient/LineBodyHandlerTest.java fail infrequently with java.net.ConnectException: Connection timed out: no further information

2020-12-09 Thread Chris Hegarty 
On Wed, 9 Dec 2020 18:50:50 GMT, Daniel Fuchs  wrote:

>> Hi, 
>> 
>> Please find here a changeset that fixes the infrequent (but annoying) test 
>> failures
>> caused by unexpected ConnectionException "Connection timed out: no further 
>> information"
>> which have been observed to occur on some platforms.
>> 
>> Tests are updated to allow the test server to handle requests concurrently.
>> PlainHttpConnection is updated to retry connection once if 
>> chan::finishConnect fails
>> early with ConnectionException and the connection timeout has not expired.
>
> Daniel Fuchs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8256459: java/net/httpclient/ManyRequests.java and 
> java/net/httpclient/LineBodyHandlerTest.java fail infrequently with 
> java.net.ConnectException: Connection timed out: no further information
>   
>   Connect will not be retried if retry connect is disabled
>   JBS issue label noreg-self removed and bug id added to tests

Marked as reviewed by chegar (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/1716