Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer

2022-05-10 Thread Xue-Lei Andrew Fan
On Fri, 29 Apr 2022 03:58:57 GMT, Anthony Scarpino  
wrote:

> Hi,
> 
> I need a review of this fix to allow a read-only 'src' buffer to be used with 
> SSLEngine.unwrap(). A temporary read-write buffer is created in the SSLCipher 
> operation when a read-only buffer is passed. If the 'src' is read-write, 
> there is no effect on the current operation
> 
> The PR also includes a CSR for an API implementation note to the 
> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption 
> operation. 'unwrap()' has had this behavior forever, so there is no 
> compatibility issue with this note. Using the 'src' buffer for in-place 
> decryption was a performance decision.
> 
> Tony

src/java.base/share/classes/javax/net/ssl/SSLEngine.java line 677:

> 675:  * @see #unwrap(ByteBuffer, ByteBuffer[], int, int)
> 676:  *
> 677:  * @implNote The data in {@code src} may be modified during the 
> decryption

It looks like a note for the API users to me.  Is apiNote tag more appropriate 
here?

-

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


Re: RFR: 8286378: Address possibly lossy conversions in java.base [v2]

2022-05-10 Thread Alan Bateman
On Tue, 10 May 2022 23:01:33 GMT, Roger Riggs  wrote:

>> PR#8599 8244681: proposes to add compiler warnings for possible lossy 
>> conversions
>> From the CSR:
>> 
>> "If the type of the right-hand operand of a compound assignment is not 
>> assignment compatible with the type of the variable, a cast is implied and 
>> possible lossy conversion may silently occur. While similar situations are 
>> resolved as compilation errors for primitive assignments, there are no 
>> similar rules defined for compound assignments."
>> 
>> This PR anticipates the warnings and adds explicit casts to replace the 
>> implicit casts.
>> In most cases, the cast matches the type of the right-hand side to type of 
>> the compound assignment.
>> Since these casts are truncating the value, there might be a different 
>> refactoring that avoids the cast
>> but a direct replacement was chosen here.
>> 
>> (Advise and suggestions will inform similar updates to other OpenJDK 
>> modules).
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   remove stray file

src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java line 128:

> 126: // timed poll interrupted so need to adjust timeout
> 127: long adjust = System.nanoTime() - startTime;
> 128: to -= (int)TimeUnit.MILLISECONDS.convert(adjust, 
> TimeUnit.NANOSECONDS);

Can we change this `to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust);` while 
we here?

src/java.base/share/classes/jdk/internal/org/objectweb/asm/Frame.java line 615:

> 613: if (outputStackTop >= elements) {
> 614: outputStackTop -= (short)elements;
> 615: } else {

I think we usually try to avoid changing the ASM code if possible but maybe you 
have to do this because the compiler option is used for compiling java.base?

src/java.base/unix/classes/sun/nio/ch/PollSelectorImpl.java line 123:

> 121: // timed poll interrupted so need to adjust timeout
> 122: long adjust = System.nanoTime() - startTime;
> 123: to -= (int)TimeUnit.MILLISECONDS.convert(adjust, 
> TimeUnit.NANOSECONDS);

This one can also be changed to:

`to =- (int) TimeUnit.NANOSECONDS.toMillis(adjust);`

-

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


Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer

2022-05-10 Thread Bradford Wetmore
On Fri, 29 Apr 2022 03:58:57 GMT, Anthony Scarpino  
wrote:

> Hi,
> 
> I need a review of this fix to allow a read-only 'src' buffer to be used with 
> SSLEngine.unwrap(). A temporary read-write buffer is created in the SSLCipher 
> operation when a read-only buffer is passed. If the 'src' is read-write, 
> there is no effect on the current operation
> 
> The PR also includes a CSR for an API implementation note to the 
> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption 
> operation. 'unwrap()' has had this behavior forever, so there is no 
> compatibility issue with this note. Using the 'src' buffer for in-place 
> decryption was a performance decision.
> 
> Tony

All of the comments observed are minor.  

Please consider some of the test changes, but otherwise looks good.

-

Marked as reviewed by wetmore (Reviewer).

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


Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer

2022-05-10 Thread Bradford Wetmore
On Fri, 29 Apr 2022 03:58:57 GMT, Anthony Scarpino  
wrote:

> Hi,
> 
> I need a review of this fix to allow a read-only 'src' buffer to be used with 
> SSLEngine.unwrap(). A temporary read-write buffer is created in the SSLCipher 
> operation when a read-only buffer is passed. If the 'src' is read-write, 
> there is no effect on the current operation
> 
> The PR also includes a CSR for an API implementation note to the 
> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption 
> operation. 'unwrap()' has had this behavior forever, so there is no 
> compatibility issue with this note. Using the 'src' buffer for in-place 
> decryption was a performance decision.
> 
> Tony

Finished the SSLEngine/tests, starting on the SSLCipher.  Here's the notes so 
far.

test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 1:

> 1: /*

Wondering why this is in javax/net/ssl/SSLSession instead of 
sun/security/ssl/SSLCipher.

test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 51:

> 49: public class ReadOnlyEngine {
> 50: 
> 51: private static String pathToStores = "../etc";

These 6 can be final if you want.

test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 96:

> 94: status = engine.getHandshakeStatus();
> 95: break;
> 96: case FINISHED:

Can combine FINISHED/NOT_HANDSHAKING?

test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 157:

> 155: // Do TLS handshake
> 156: do {
> 157: statusClient = doHandshake(client, out, in);

It's potentially a little inefficient returning after each wrap/unwrap() 
instead of doing the task right away, but it works.  No need to change.

Is this style something you copied from another test?  The  SSLEngineTemplate 
in the templates directory is what I often use.

test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 160:

> 158: statusServer = doHandshake(server, in, out);
> 159: } while (statusClient != HandshakeStatus.NOT_HANDSHAKING ||
> 160: statusServer != HandshakeStatus.NOT_HANDSHAKING);

Minor indent problem.

test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 162:

> 160: statusServer != HandshakeStatus.NOT_HANDSHAKING);
> 161: 
> 162: // Read NST

What is NST?

test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 172:

> 170: out.clear();
> 171: String testString = "ASDF";
> 172: in.put(testString.getBytes()).flip();

If you're going to convert back from UTF_8 later, you should probably convert 
using getBytes(UTF_8) here.

test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 188:

> 186: in.clear();
> 187: System.out.println("2: Server send: " + testString);
> 188: in.put(testString.getBytes()).flip();

Same

test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 189:

> 187: System.out.println("2: Server send: " + testString);
> 188: in.put(testString.getBytes()).flip();
> 189: send(server, in, out);

Did you want to try asReadOnlyBuffer() here also?

test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 191:

> 189: send(server, in, out);
> 190: in.clear();
> 191: receive(client, out, in);

And here?

-

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


Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer

2022-05-10 Thread Mark Powers
On Fri, 29 Apr 2022 03:58:57 GMT, Anthony Scarpino  
wrote:

> Hi,
> 
> I need a review of this fix to allow a read-only 'src' buffer to be used with 
> SSLEngine.unwrap(). A temporary read-write buffer is created in the SSLCipher 
> operation when a read-only buffer is passed. If the 'src' is read-write, 
> there is no effect on the current operation
> 
> The PR also includes a CSR for an API implementation note to the 
> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption 
> operation. 'unwrap()' has had this behavior forever, so there is no 
> compatibility issue with this note. Using the 'src' buffer for in-place 
> decryption was a performance decision.
> 
> Tony

I don't yet count as a reviewer, but your changes look good to me. You might 
want to let IntelliJ check for problems before you integrate.

-

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


Re: RFR: 8286378: Address possibly lossy conversions in java.base [v2]

2022-05-10 Thread Roger Riggs
> PR#8599 8244681: proposes to add compiler warnings for possible lossy 
> conversions
> From the CSR:
> 
> "If the type of the right-hand operand of a compound assignment is not 
> assignment compatible with the type of the variable, a cast is implied and 
> possible lossy conversion may silently occur. While similar situations are 
> resolved as compilation errors for primitive assignments, there are no 
> similar rules defined for compound assignments."
> 
> This PR anticipates the warnings and adds explicit casts to replace the 
> implicit casts.
> In most cases, the cast matches the type of the right-hand side to type of 
> the compound assignment.
> Since these casts are truncating the value, there might be a different 
> refactoring that avoids the cast
> but a direct replacement was chosen here.
> 
> (Advise and suggestions will inform similar updates to other OpenJDK modules).

Roger Riggs has updated the pull request incrementally with one additional 
commit since the last revision:

  remove stray file

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8642/files
  - new: https://git.openjdk.java.net/jdk/pull/8642/files/e72ce85c..7ff806a5

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8642=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8642=00-01

  Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8642.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8642/head:pull/8642

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


Re: RFR: 8286378: Address possibly lossy conversions in java.base

2022-05-10 Thread Brian Burkhalter
On Tue, 10 May 2022 21:32:10 GMT, Roger Riggs  wrote:

> PR#8599 8244681: proposes to add compiler warnings for possible lossy 
> conversions
> From the CSR:
> 
> "If the type of the right-hand operand of a compound assignment is not 
> assignment compatible with the type of the variable, a cast is implied and 
> possible lossy conversion may silently occur. While similar situations are 
> resolved as compilation errors for primitive assignments, there are no 
> similar rules defined for compound assignments."
> 
> This PR anticipates the warnings and adds explicit casts to replace the 
> implicit casts.
> In most cases, the cast matches the type of the right-hand side to type of 
> the compound assignment.
> Since these casts are truncating the value, there might be a different 
> refactoring that avoids the cast
> but a direct replacement was chosen here.
> 
> (Advise and suggestions will inform similar updates to other OpenJDK modules).

IO, math, and NIO changes look fine.

-

Marked as reviewed by bpb (Reviewer).

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


Re: RFR: 8286378: Address possibly lossy conversions in java.base

2022-05-10 Thread Xue-Lei Andrew Fan
On Tue, 10 May 2022 21:32:10 GMT, Roger Riggs  wrote:

> PR#8599 8244681: proposes to add compiler warnings for possible lossy 
> conversions
> From the CSR:
> 
> "If the type of the right-hand operand of a compound assignment is not 
> assignment compatible with the type of the variable, a cast is implied and 
> possible lossy conversion may silently occur. While similar situations are 
> resolved as compilation errors for primitive assignments, there are no 
> similar rules defined for compound assignments."
> 
> This PR anticipates the warnings and adds explicit casts to replace the 
> implicit casts.
> In most cases, the cast matches the type of the right-hand side to type of 
> the compound assignment.
> Since these casts are truncating the value, there might be a different 
> refactoring that avoids the cast
> but a direct replacement was chosen here.
> 
> (Advise and suggestions will inform similar updates to other OpenJDK modules).

The update in security area looks good to me.

-

Marked as reviewed by xuelei (Reviewer).

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


Re: RFR: 8286378: Address possibly lossy conversions in java.base

2022-05-10 Thread Naoto Sato
On Tue, 10 May 2022 21:32:10 GMT, Roger Riggs  wrote:

> PR#8599 8244681: proposes to add compiler warnings for possible lossy 
> conversions
> From the CSR:
> 
> "If the type of the right-hand operand of a compound assignment is not 
> assignment compatible with the type of the variable, a cast is implied and 
> possible lossy conversion may silently occur. While similar situations are 
> resolved as compilation errors for primitive assignments, there are no 
> similar rules defined for compound assignments."
> 
> This PR anticipates the warnings and adds explicit casts to replace the 
> implicit casts.
> In most cases, the cast matches the type of the right-hand side to type of 
> the compound assignment.
> Since these casts are truncating the value, there might be a different 
> refactoring that avoids the cast
> but a direct replacement was chosen here.
> 
> (Advise and suggestions will inform similar updates to other OpenJDK modules).

Looks good. Assuming copyright years will be updated.

src/java.base/share/classes/java/time/es line 1:

> 1: X

Looks like a random file was added by accident?

-

Marked as reviewed by naoto (Reviewer).

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


RFR: 8286378: Address possibly lossy conversions in java.base

2022-05-10 Thread Roger Riggs
PR#8599 8244681: proposes to add compiler warnings for possible lossy 
conversions
>From the CSR:

"If the type of the right-hand operand of a compound assignment is not 
assignment compatible with the type of the variable, a cast is implied and 
possible lossy conversion may silently occur. While similar situations are 
resolved as compilation errors for primitive assignments, there are no similar 
rules defined for compound assignments."

This PR anticipates the warnings and adds explicit casts to replace the 
implicit casts.
In most cases, the cast matches the type of the right-hand side to type of the 
compound assignment.
Since these casts are truncating the value, there might be a different 
refactoring that avoids the cast
but a direct replacement was chosen here.

(Advise and suggestions will inform similar updates to other OpenJDK modules).

-

Commit messages:
 - 8286378: Address possibly lossy conversions in java.base

Changes: https://git.openjdk.java.net/jdk/pull/8642/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8642=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8286378
  Stats: 57 lines in 33 files changed: 1 ins; 3 del; 53 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8642.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8642/head:pull/8642

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


Re: RFR: JDK-6725221 Standardize obtaining boolean properties with defaults

2022-05-10 Thread Mark Powers
On Fri, 6 May 2022 17:56:44 GMT, Alan Bateman  wrote:

>> JDK-6725221 Standardize obtaining boolean properties with defaults
>
> src/java.base/share/classes/java/lang/reflect/AccessibleObject.java line 777:
> 
>> 775: if (!printStackPropertiesSet && VM.initLevel() >= 1) {
>> 776: printStackWhenAccessFails = GetBooleanAction.
>> 777: 
>> privilegedGetProperty("sun.reflect.debugModuleAccessChecks");
> 
> Running with `-Dsun.reflect.debugModuleAccessChecks` should set 
> printStackWhenAccessFails to true, not false.

You are right. The old way maps the null string to true, and the new way maps 
it to false. I did not notice that. At this point, I see no value in making the 
"true".equals and "false".equals changes. Too much can break. I'll reverse 
these changes.

-

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


Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v19]

2022-05-10 Thread Daniel Fuchs
On Tue, 10 May 2022 13:51:37 GMT, Jaikiran Pai  wrote:

>> This change proposes to implement the enhancement noted in 
>> https://bugs.openjdk.java.net/browse/JDK-8209137.
>> 
>> The change introduces a new API to allow applications to build a 
>> `java.net.http.HTTPClient` configured with a specific local address that 
>> will be used while creating `Socket`(s) for connections.
>
> Jaikiran Pai has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fix javadoc link in test

LGTM

-

Marked as reviewed by dfuchs (Reviewer).

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


Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v18]

2022-05-10 Thread Jaikiran Pai
On Tue, 10 May 2022 13:35:35 GMT, Daniel Fuchs  wrote:

>> Jaikiran Pai has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Daniel's review suggestion - add a test to verify the behaviour of the 
>> localAddress() default method implementation on HttpClient.Builder
>>  - Daniel's review suggestion - remove reference to "Internet Protocol" in 
>> javadoc
>
> test/jdk/java/net/httpclient/HttpClientBuilderTest.java line 268:
> 
>> 266: /**
>> 267:  * Tests the {@link 
>> HttpClient,java.net.http.HttpClient.Builder#localAddress(InetAddress)} method
>> 268:  * behaviour when that method is called on a builder returned by 
>> {@link HttpClient#newBuilder()}
> 
> /**
>   * Tests the {@link 
> HttpClient,java.net.http.HttpClient.Builder#localAddress(InetAddress)} method
>   ```
>   This `{@link` looks broken - the `HttpClient,` prefix probably need to be 
> removed?

Indeed. That prefix wasn't intentional. I'm not sure how I ended up with that. 
I just pushed an update to fix this. Thank you for flagging this.

-

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


Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v18]

2022-05-10 Thread Jaikiran Pai
On Tue, 10 May 2022 12:37:47 GMT, Jaikiran Pai  wrote:

>> This change proposes to implement the enhancement noted in 
>> https://bugs.openjdk.java.net/browse/JDK-8209137.
>> 
>> The change introduces a new API to allow applications to build a 
>> `java.net.http.HTTPClient` configured with a specific local address that 
>> will be used while creating `Socket`(s) for connections.
>
> Jaikiran Pai has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Daniel's review suggestion - add a test to verify the behaviour of the 
> localAddress() default method implementation on HttpClient.Builder
>  - Daniel's review suggestion - remove reference to "Internet Protocol" in 
> javadoc

I've triggered some internal CI runs against the current state of PR to see all 
works fine. Now that we have decided on the nature of this API, I'll go ahead 
and file a CSR for this.

-

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


Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v19]

2022-05-10 Thread Jaikiran Pai
> This change proposes to implement the enhancement noted in 
> https://bugs.openjdk.java.net/browse/JDK-8209137.
> 
> The change introduces a new API to allow applications to build a 
> `java.net.http.HTTPClient` configured with a specific local address that will 
> be used while creating `Socket`(s) for connections.

Jaikiran Pai has updated the pull request incrementally with one additional 
commit since the last revision:

  fix javadoc link in test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6690/files
  - new: https://git.openjdk.java.net/jdk/pull/6690/files/3a6f2b6b..d9f7b077

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=6690=18
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6690=17-18

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/6690.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/6690/head:pull/6690

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


Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v18]

2022-05-10 Thread Daniel Fuchs
On Tue, 10 May 2022 13:36:18 GMT, Daniel Fuchs  wrote:

>> Jaikiran Pai has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Daniel's review suggestion - add a test to verify the behaviour of the 
>> localAddress() default method implementation on HttpClient.Builder
>>  - Daniel's review suggestion - remove reference to "Internet Protocol" in 
>> javadoc
>
> test/jdk/java/net/httpclient/HttpClientBuilderTest.java line 283:
> 
>> 281:  * Tests that the default method implementation of
>> 282:  * {@link 
>> HttpClient,java.net.http.HttpClient.Builder#localAddress(InetAddress)} throws
>> 283:  * an {@link UnsupportedOperationException}
> 
> Same remark here

Otherwise things look good to me - you should update the CSR if it needs to be 
updated.

-

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


Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v18]

2022-05-10 Thread Daniel Fuchs
On Tue, 10 May 2022 12:37:47 GMT, Jaikiran Pai  wrote:

>> This change proposes to implement the enhancement noted in 
>> https://bugs.openjdk.java.net/browse/JDK-8209137.
>> 
>> The change introduces a new API to allow applications to build a 
>> `java.net.http.HTTPClient` configured with a specific local address that 
>> will be used while creating `Socket`(s) for connections.
>
> Jaikiran Pai has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Daniel's review suggestion - add a test to verify the behaviour of the 
> localAddress() default method implementation on HttpClient.Builder
>  - Daniel's review suggestion - remove reference to "Internet Protocol" in 
> javadoc

test/jdk/java/net/httpclient/HttpClientBuilderTest.java line 268:

> 266: /**
> 267:  * Tests the {@link 
> HttpClient,java.net.http.HttpClient.Builder#localAddress(InetAddress)} method
> 268:  * behaviour when that method is called on a builder returned by 
> {@link HttpClient#newBuilder()}

/**
  * Tests the {@link 
HttpClient,java.net.http.HttpClient.Builder#localAddress(InetAddress)} method
  ```
  This `{@link` looks broken - the `HttpClient,` prefix probably need to be 
removed?

test/jdk/java/net/httpclient/HttpClientBuilderTest.java line 283:

> 281:  * Tests that the default method implementation of
> 282:  * {@link 
> HttpClient,java.net.http.HttpClient.Builder#localAddress(InetAddress)} throws
> 283:  * an {@link UnsupportedOperationException}

Same remark here

-

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


Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v17]

2022-05-10 Thread Jaikiran Pai
On Mon, 9 May 2022 13:31:38 GMT, Daniel Fuchs  wrote:

>> Jaikiran Pai has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 37 commits:
>> 
>>  - Merge latest from master branch
>>  - add a @build to force jtreg to show consistent test results and add the 
>> relevant permissions for security manager testing
>>  - Change the new API to accept an InetAddress instead of an 
>> InetSocketAddress, after inputs from Michael, Daniel and others
>>  - Merge latest from master
>>  - Implement HttpServerAdapters in test as suggested by Daniel
>>  - fix check when security manager is enabled
>>  - Add a unit test for the new HttpClient.Builder.localAddress method
>>  - Implement Daniel's suggestion - only support InetSocketAddress with port 0
>>  - Merge latest from master branch
>>  - Merge latest from master branch
>>  - ... and 27 more: 
>> https://git.openjdk.java.net/jdk/compare/b490a58e...d4a19dea
>
> src/java.net.http/share/classes/java/net/http/HttpClient.java line 398:
> 
>> 396:  *
>> 397:  * @implSpec If the {@link #localAddress(InetAddress) local 
>> address} is a non-null
>> 398:  * Internet Protocol address and a security manager is 
>> installed, then
> 
> Here and in the @throws bellow we could now remove mention of `Internet 
> Protocol`.
> For instance, here we could say 
> 
> ... is a non-null address and a security ...

Done. Updated this and the other javadoc comment to follow this suggestion.

> test/jdk/java/net/httpclient/HttpClientBuilderTest.java line 274:
> 
>> 272: // setting null should work fine
>> 273: builder.localAddress(null);
>> 274: builder.localAddress(InetAddress.getLoopbackAddress());
> 
> We probably also need a MockHttpClientBuilder to test the behaviour of the 
> default implementation (check that it throws UOE).

I've now updated this PR to add a new test method which tests the default 
method implementation of `localAddress`.

-

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


Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v18]

2022-05-10 Thread Jaikiran Pai
> This change proposes to implement the enhancement noted in 
> https://bugs.openjdk.java.net/browse/JDK-8209137.
> 
> The change introduces a new API to allow applications to build a 
> `java.net.http.HTTPClient` configured with a specific local address that will 
> be used while creating `Socket`(s) for connections.

Jaikiran Pai has updated the pull request incrementally with two additional 
commits since the last revision:

 - Daniel's review suggestion - add a test to verify the behaviour of the 
localAddress() default method implementation on HttpClient.Builder
 - Daniel's review suggestion - remove reference to "Internet Protocol" in 
javadoc

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6690/files
  - new: https://git.openjdk.java.net/jdk/pull/6690/files/d4a19dea..3a6f2b6b

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=6690=17
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6690=16-17

  Stats: 74 lines in 2 files changed: 70 ins; 2 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/6690.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/6690/head:pull/6690

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