Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Wed, 11 May 2022 22:49:02 GMT, Anthony Scarpino
wrote:
>> 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?
>
> The CSR and this code is changing, the note I was adding was already
> documented, but its existing wording should be more clear.
I like the new update as specified in the CSR. I added my name as Reviewer for
the CSR.
-
PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Wed, 11 May 2022 00:31:23 GMT, Bradford Wetmore 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
>
> 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.
setting the input to UTF8 isn't a concern. The latter line to decode it
changes it from using the ByteBuffer.toString() to the contents of the
ByteBuffer in a String.
> 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
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?
Yeah, they should have been. I must have undid it to test something
> 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?
Same
-
PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Wed, 11 May 2022 05:52:38 GMT, Xue-Lei Andrew Fan 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?
The CSR and this code is changing, the note I was adding was already
documented, but its existing wording should be more clear.
-
PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Mon, 9 May 2022 23:48:24 GMT, Bradford Wetmore 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
>
> 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.
I got it from somewhere that I don't remember off hand. I'm just trying to get
through the handshake.
> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 162:
>
>> 160: statusServer != HandshakeStatus.NOT_HANDSHAKING);
>> 161:
>> 162: // Read NST
>
> What is NST?
New Session Ticket
-
PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Mon, 9 May 2022 23:15:40 GMT, Bradford Wetmore 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 > > 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. I can move it.. I created it from another test which happen to be in that directory - PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Wed, 11 May 2022 17:10:45 GMT, Daniel Fuchs wrote: >> src/java.net.http/share/classes/jdk/internal/net/http/hpack/QuickHuffman.java >> line 739: >> >>> 737: buffer |= (codeValueOf(c) >>> bufferLen); // >>> append >>> 738: bufferLen += (int) len; >>> 739: pos++; >> >> Would it be cleaner to do the cast in the codeLengthOf method, so it returns >> an int? > > I believe the method returns an "unsigned int" - having the method return an > int would then potentially cause `bufferLen + len <= 64` to yield true when > it shouldn't. Hopefully @pavelrappo will comment. codeLengthOf() returns long. It could be changed to return int with a cast internally and then you could avoid the two new casts. - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v3]
On Wed, 11 May 2022 16:30:41 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: > > Updated copyrights > Fixed cast style to add a space after cast, (where consistent with file > style) > Improved code per review comments in PollSelectors. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Revert adding char constants - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/fbf3c9a1..2334b747 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8656=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8656=03-04 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v4]
On Wed, 11 May 2022 18:25:00 GMT, Daniel Fuchs wrote: >> @RogerRiggs Actually - I'm no longer sure that this will work: >> >> >> jshell> char x = 0b1000_ >> x ==> '耀' >> >> jshell> var y = ~x >> y ==> -32769 >> >> jshell> char y = ~x >> | Error: >> | incompatible types: possible lossy conversion from int to char >> | char y = ~x; >> | ^^ > > So if x is a char, ~x seems to be an int :-( I have reverted adding constant fields. Too bad - the casts are back. - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v4]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Revert adding char constants - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/ec344eef..fbf3c9a1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8656=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8656=02-03 Stats: 29 lines in 1 file changed: 15 ins; 3 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v3]
On Wed, 11 May 2022 18:23:30 GMT, Daniel Fuchs wrote: >> I'd put `_MASK` in the name along with whatever name is used for the bits >> in the frame spec . > > @RogerRiggs Actually - I'm no longer sure that this will work: > > > jshell> char x = 0b1000_ > x ==> '耀' > > jshell> var y = ~x > y ==> -32769 > > jshell> char y = ~x > | Error: > | incompatible types: possible lossy conversion from int to char > | char y = ~x; > | ^^ So if x is a char, ~x seems to be an int :-( - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v3]
On Wed, 11 May 2022 17:49:28 GMT, Roger Riggs wrote: >> Ah! Good point. Maybe I should use a constant and get rid of the cast. > > I'd put `_MASK` in the name along with whatever name is used for the bits > in the frame spec . @RogerRiggs Actually - I'm no longer sure that this will work: jshell> char x = 0b1000_ x ==> '耀' jshell> var y = ~x y ==> -32769 jshell> char y = ~x | Error: | incompatible types: possible lossy conversion from int to char | char y = ~x; | ^^ - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v3]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Add _MASK suffix to char constants - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/bbcf238b..ec344eef Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8656=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8656=01-02 Stats: 14 lines in 1 file changed: 0 ins; 0 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v2]
On Wed, 11 May 2022 17:49:28 GMT, Roger Riggs wrote: >> Ah! Good point. Maybe I should use a constant and get rid of the cast. > > I'd put `_MASK` in the name along with whatever name is used for the bits > in the frame spec . Done - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v2]
On Wed, 11 May 2022 18:02:32 GMT, Daniel Fuchs wrote: >> In relation to >> [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find >> here a patch that addresses possibly lossy conversions in java.net.http > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > Use char constant to avoid casts LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v2]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Use char constant to avoid casts - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/6ef906e0..bbcf238b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8656=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8656=00-01 Stats: 29 lines in 1 file changed: 5 ins; 15 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http
On Wed, 11 May 2022 17:37:44 GMT, Roger Riggs wrote:
>> Yes - that's my understanding.
>
> These methods either set or clear a single bit in `firstChar`.
> The constant is an `int` so its complement is a 32 bit int.
>
> It could also be written as `~ (char) 0b100_000`. Then the 16 bit
> unsigned char would be complemented.
> Either way, the cast is needed to be explicit about the size of the constant.
>
> Another way to avoid the cast would be to define the bit positions as:
>
> `private static final char FIN_BIT = 0b1000_;
> ... etc.
> `
> Then the code in the methods would not need the cast.
>
>
> if (value) {
> firstChar |= FIN_BIT;
> } else {
> firstChar &= ~FIN_BIT;
> }
Ah! Good point. Maybe I should use a constant and get rid of the cast.
-
PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http
On Wed, 11 May 2022 17:46:15 GMT, Daniel Fuchs wrote:
>> These methods either set or clear a single bit in `firstChar`.
>> The constant is an `int` so its complement is a 32 bit int.
>>
>> It could also be written as `~ (char) 0b100_000`. Then the 16 bit
>> unsigned char would be complemented.
>> Either way, the cast is needed to be explicit about the size of the constant.
>>
>> Another way to avoid the cast would be to define the bit positions as:
>>
>> `private static final char FIN_BIT = 0b1000_;
>> ... etc.
>> `
>> Then the code in the methods would not need the cast.
>>
>>
>> if (value) {
>> firstChar |= FIN_BIT;
>> } else {
>> firstChar &= ~FIN_BIT;
>> }
>
> Ah! Good point. Maybe I should use a constant and get rid of the cast.
I'd put `_MASK` in the name along with whatever name is used for the bits in
the frame spec .
-
PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http
On Wed, 11 May 2022 17:04:19 GMT, Daniel Fuchs wrote:
>> src/java.net.http/share/classes/jdk/internal/net/http/websocket/Frame.java
>> line 222:
>>
>>> 220: // compiler will emit a warning if not cast
>>> 221: firstChar &= (char) ~0b1000_;
>>> 222: }
>>
>> Am I right in believing that there was an implicit cast to char here before
>> and the only change is to make it explicit? ie. there is no actual behavior
>> change?
>
> Yes - that's my understanding.
These methods either set or clear a single bit in `firstChar`.
The constant is an `int` so its complement is a 32 bit int.
It could also be written as `~ (char) 0b100_000`. Then the 16 bit
unsigned char would be complemented.
Either way, the cast is needed to be explicit about the size of the constant.
Another way to avoid the cast would be to define the bit positions as:
`private static final char FIN_BIT = 0b1000_;
... etc.
`
Then the code in the methods would not need the cast.
if (value) {
firstChar |= FIN_BIT;
} else {
firstChar &= ~FIN_BIT;
}
-
PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http
On Wed, 11 May 2022 16:52:07 GMT, Michael McMahon wrote: >> In relation to >> [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find >> here a patch that addresses possibly lossy conversions in java.net.http > > src/java.net.http/share/classes/jdk/internal/net/http/hpack/QuickHuffman.java > line 739: > >> 737: buffer |= (codeValueOf(c) >>> bufferLen); // >> append >> 738: bufferLen += (int) len; >> 739: pos++; > > Would it be cleaner to do the cast in the codeLengthOf method, so it returns > an int? I believe the method returns an "unsigned int" - having the method return an int would then potentially cause `bufferLen + len <= 64` to yield true when it shouldn't. Hopefully @pavelrappo will comment. - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v3]
On Wed, 11 May 2022 16:30:41 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: > > Updated copyrights > Fixed cast style to add a space after cast, (where consistent with file > style) > Improved code per review comments in PollSelectors. Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http
On Wed, 11 May 2022 16:55:16 GMT, Michael McMahon wrote: >> In relation to >> [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find >> here a patch that addresses possibly lossy conversions in java.net.http > > src/java.net.http/share/classes/jdk/internal/net/http/websocket/Frame.java > line 222: > >> 220: // compiler will emit a warning if not cast >> 221: firstChar &= (char) ~0b1000_; >> 222: } > > Am I right in believing that there was an implicit cast to char here before > and the only change is to make it explicit? ie. there is no actual behavior > change? Yes - that's my understanding. - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http
On Wed, 11 May 2022 15:42:25 GMT, Daniel Fuchs wrote: > In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http src/java.net.http/share/classes/jdk/internal/net/http/hpack/QuickHuffman.java line 739: > 737: buffer |= (codeValueOf(c) >>> bufferLen); // > append > 738: bufferLen += (int) len; > 739: pos++; Would it be cleaner to do the cast in the codeLengthOf method, so it returns an int? src/java.net.http/share/classes/jdk/internal/net/http/websocket/Frame.java line 222: > 220: // compiler will emit a warning if not cast > 221: firstChar &= (char) ~0b1000_; > 222: } Am I right in believing that there was an implicit cast to char here before and the only change is to make it explicit? ie. there is no actual behavior change? - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v3]
> 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: Updated copyrights Fixed cast style to add a space after cast, (where consistent with file style) Improved code per review comments in PollSelectors. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8642/files - new: https://git.openjdk.java.net/jdk/pull/8642/files/7ff806a5..a6679ce7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8642=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8642=01-02 Stats: 41 lines in 24 files changed: 0 ins; 0 del; 41 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: 8286386: Address possibly lossy conversions in java.net.http
On Wed, 11 May 2022 15:42:25 GMT, Daniel Fuchs wrote: > In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http @pavelrappo I would appreciate if you could review these changes - PR: https://git.openjdk.java.net/jdk/pull/8656
RFR: 8286386: Address possibly lossy conversions in java.net.http
In relation to [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find here a patch that addresses possibly lossy conversions in java.net.http - Commit messages: - Fix comments - 8286386: Address possibly lossy conversions in java.net.http Changes: https://git.openjdk.java.net/jdk/pull/8656/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8656=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286386 Stats: 27 lines in 3 files changed: 15 ins; 0 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v19]
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 I've now created a CSR for this change https://bugs.openjdk.java.net/browse/JDK-8286583 - PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v19]
On Wed, 11 May 2022 11:34:33 GMT, Michael McMahon wrote: >> Hello Michael, >> Most users will be using the `HttpClient.newBuilder()` to create the >> builder, so this note about `UnsupportedOperationException` can be >> confusing. However, for implementations (libraries?) which provide their >> own implementation of the `java.net.http.HttpClient.Builder`, I think, this >> note would be relevant. This approach is similar to what we already have on >> `java.net.http.HttpClient.newWebSocketBuilder()` method. > > Sure, I just think when most developers read "The default implementation of > this method throws UOE" they will think they can't use it without > implementing something themselves. Library developers will figure it out > anyway. This is the standard wording that has been used throughout the JDK to document the behavior of default methods on interfaces. Unless we receive different feedback during the CSR review I'd suggest to leave it that way. The second sentence makes it clear that our concrete implementations override that default behavior. - PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v19]
On Wed, 11 May 2022 08:54:48 GMT, Jaikiran Pai wrote:
>> src/java.net.http/share/classes/java/net/http/HttpClient.java line 378:
>>
>>> 376: *
>>> 377: * @implSpec The default implementation of this method throws
>>> 378: * {@code UnsupportedOperationException}. {@code Builder}s
>>> obtained
>>
>> I think the implSpec here is correct, but will be confusing for most users.
>> I'm not sure what value it adds. Do we really need it?
>
> Hello Michael,
> Most users will be using the `HttpClient.newBuilder()` to create the builder,
> so this note about `UnsupportedOperationException` can be confusing. However,
> for implementations (libraries?) which provide their own implementation of
> the `java.net.http.HttpClient.Builder`, I think, this note would be relevant.
> This approach is similar to what we already have on
> `java.net.http.HttpClient.newWebSocketBuilder()` method.
Sure, I just think when most developers read "The default implementation of
this method throws UOE" they will think they can't use it without implementing
something themselves. Library developers will figure it out anyway.
-
PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v19]
On Wed, 11 May 2022 08:12:12 GMT, Michael McMahon wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix javadoc link in test
>
> src/java.net.http/share/classes/java/net/http/HttpClient.java line 378:
>
>> 376: *
>> 377: * @implSpec The default implementation of this method throws
>> 378: * {@code UnsupportedOperationException}. {@code Builder}s
>> obtained
>
> I think the implSpec here is correct, but will be confusing for most users.
> I'm not sure what value it adds. Do we really need it?
Hello Michael,
Most users will be using the `HttpClient.newBuilder()` to create the builder,
so this note about `UnsupportedOperationException` can be confusing. However,
for implementations (libraries?) which provide their own implementation of the
`java.net.http.HttpClient.Builder`, I think, this note would be relevant. This
approach is similar to what we already have on
`java.net.http.HttpClient.newWebSocketBuilder()` method.
-
PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v19]
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
src/java.net.http/share/classes/java/net/http/HttpClient.java line 378:
> 376: *
> 377: * @implSpec The default implementation of this method throws
> 378: * {@code UnsupportedOperationException}. {@code Builder}s
> obtained
I think the implSpec here is correct, but will be confusing for most users. I'm
not sure what value it adds. Do we really need it?
-
PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8286378: Address possibly lossy conversions in java.base [v2]
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 I can confirm this patch clears all warnings from java.base. - PR: https://git.openjdk.java.net/jdk/pull/8642
