On Fri, 27 Feb 2026 14:26:19 GMT, Daniel Fuchs <[email protected]> wrote:
>> src/java.net.http/share/classes/jdk/internal/net/http/quic/PeerConnectionId.java
>> line 77:
>>
>>> 75: final byte[] idBytes = new byte[src.remaining()];
>>> 76: src.get(src.position(), idBytes);
>>> 77: return ByteBuffer.wrap(idBytes);
>>
>> You can alternatively consider replacing these 3 lines with `return
>> src.slice(src.position(), src.limit())`, I guess.
>
> Not exactly as that would retain the memory from the input buffer which might
> be large (could be a slice of the whole packet).
Okay, I see — would be nice to document this in a comment. It might be me, but
this crucial detail is very easy to miss.
>> src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicConnectionImpl.java
>> line 1949:
>>
>>> 1947: if (t instanceof AssertionError) {
>>> 1948:
>>> this.terminator.terminate(TerminationCause.forException(t));
>>> 1949: }
>>
>> Would you mind explaining the rationale for this change, please?
>
> This is to make tests fail if the assertion is fired. Typically an exception
> while decoding/parsing would just cause the packet to be skipped. An
> assertion here indicates that invariants have been violated (a problem in our
> own code which should never happen), not that bad/unexpected bytes were
> received. If that happens (assert fired), we want to know about it.
> Assertions are typically enabled in testing (they are when running JDK tests)
> but not in production.
I see. I'm assuming this is the most economic — in terms of, not just LoC, but
also complexity — way to inform tests about this unexpected condition.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29956#discussion_r2864692470
PR Review Comment: https://git.openjdk.org/jdk/pull/29956#discussion_r2864692247
