On Mon, 28 Jul 2025 06:56:38 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Volkan Yazici has updated the pull request incrementally with six additional >> commits since the last revision: >> >> - Fix typo in `Utils::getBufferWithAtMost` Javadoc >> - Link to `BUFSIZE` in `Utils::getBuffer` Javadoc >> - Improve wording for signaling request cancellation >> - Remove synchronization for `FileChannelIterator` >> - Improve exception handling and documentation for `ofFileChannel` >> - Add `@since 26` to `ofFileChannel` > > test/jdk/java/net/httpclient/FileChannelPublisherTest.java line 168: > >> 166: } >> 167: >> 168: private static BlockingQueue<byte[]> >> addRequestBodyConsumingServerHandler( > > Nit - instead of the long name, it might be better to rename the method to > just `addHandler(...)` and then add a one-liner comment to the method saying > that the handler consumes the entire request body. I'm a big fan of [self-documenting code](https://en.wikipedia.org/wiki/Self-documenting_code) over comments. Unless there is a strong objection, I'd like to stick to my long method name. 😊 > test/jdk/java/net/httpclient/FileChannelPublisherTest.java line 173: > >> 171: HttpTestHandler handler = exchange -> { >> 172: // `HttpTestExchange::toString` changes on failure, pin >> it >> 173: String exchangeName = exchange.toString(); > > Nit - the use of exchange name may not be necessary and instead we could > maybe just use the request URI (`getRequestURI()`) in the log messages. Dumping the exchange name (which includes the object identifier!) has proved in several troubleshooting occasions very helpful in this test, in particular, when request handler output can be interleaved due to either timing issues in sequential requests or concurrent requests. I settled on this version after trying out the request URI, which did not help much since it is _always_ the same. > test/jdk/java/net/httpclient/FileChannelPublisherTest.java line 198: > >> 196: "Server[%s] failed to process the request >> (exchange=%s)".formatted(serverName, exception), >> 197: exception); >> 198: readRequestBodyBytes.add(new byte[0]); > > Just to make the test a bit more robust and prevent the subsequent > `readRequestBodyBytes.take()` from blocking forever, it might be better to > either catch `Throwable` or add this `readRequestBodyBytes.add(new byte[0]);` > in the `finally` block (with some condition checks). Changed to catch `Throwable` in 99503c0bac7. > test/jdk/java/net/httpclient/FileChannelPublisherTest.java line 296: > >> 294: "6,2,5" // (offset + length) > fileSize >> 295: }) >> 296: void testIllegalOffset( > > Given what this test verifies, should we name this method > `testIllegalOffsetOrLen(...)`? Changed in 99503c0bac7. > test/jdk/java/net/httpclient/FileChannelPublisherTest.java line 309: > >> 307: @ParameterizedTest >> 308: @MethodSource("serverRequestPairs") >> 309: void testContentLessThanBufferSize( > > Hello Volkan, on first read, it's not clear to me what this test is testing. > Can a brief comment be added to this test method noting what it is testing? Good call. Improved the documentation of both `testContent{Less,More}ThanBufferSize()` methods in 99503c0bac7. > test/jdk/java/net/httpclient/FileChannelPublisherTest.java line 439: > >> 437: HttpRequest request = pair >> 438: .requestBuilder >> 439: .POST(ofFileChannel(fileChannel, fileChannelOffset, >> fileChannelLength)) > > I think it would be better to not add a static import for this method and > instead use `BodyPublishers.ofFileChannel(...)` to make it easily visible > what this method is (especially because this test has a similarly named > `withFileChannel(...)` method). Changed in 99503c0bac7. > test/jdk/java/net/httpclient/FileChannelPublisherTest.java line 513: > >> 511: LOGGER.log("Verifying the client failure"); >> 512: Exception requestFailure = >> assertThrows(ExecutionException.class, () -> responseFutureRef.get().get()); >> 513: assertInstanceOf(UncheckedIOException.class, >> requestFailure.getCause()); > > I think the application should be receiving a `IOException` (both for send() > and sendAsync()) instead of a `UncheckedIOException`. The `IOE` thrown is wrapped by an `UncheckedIOE` in `FileChannelIterator::next`, which overrides `Iterator::next` and that does not allow a `throws` in the `next()` footprint. Would you mind elaborating on your remark, please? > test/jdk/java/net/httpclient/FileChannelPublisherTest.java line 514: > >> 512: Exception requestFailure = >> assertThrows(ExecutionException.class, () -> responseFutureRef.get().get()); >> 513: assertInstanceOf(UncheckedIOException.class, >> requestFailure.getCause()); >> 514: assertInstanceOf(ClosedChannelException.class, >> requestFailure.getCause().getCause()); > > Nit - the `assertInstanceOf(...)` returns the instance of the exception which > matches the instanceof check. So it might be easier to understand while > reading this code, if we did something like: > > > final IOException ioe = assertInstanceOf(IOException.class, > requestFailure.getCause()); > assertInstanceOf(ClosedChannelException.class, ioe.getCause()); Changed as requested in 99503c0bac7. > test/jdk/java/net/httpclient/FileChannelPublisherTest.java line 528: > >> 526: // On Windows, modification while reading is not possible. >> 527: // Recall the infamous `The process cannot access the file because >> it is being used by another process`. >> 528: @DisabledOnOs(OS.WINDOWS) > > On Windows, is this still true if you replace the call in this test method > from: > > > Files.write(filePath, generateFileBytes(1)); > > to: > > fileChannel.truncate(1); Indeed this is not an issue anymore – removed cited lines in 99503c0bac7. > test/jdk/java/net/httpclient/FileChannelPublisherTest.java line 567: > >> 565: String requestFailureMessage = >> requestFailure.getMessage(); >> 566: assertTrue( >> 567: requestFailureMessage.contains("Unexpected >> EOF"), > > Wouldn't this message be on the `ExecutionException`'s `getCause()`? instead > of the `ExecutionException` itself? In fact, the message is from the `IOE` wrapped in a `UncheckedIOE` wrapped in the `ExecutionException`. I thought it would not matter from the perspective of the test. Nevertheless, in 99503c0bac7, I've updated the code to validate this causal chain and verify the message obtained from the `IOE`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/26155#discussion_r2242069117 PR Review Comment: https://git.openjdk.org/jdk/pull/26155#discussion_r2242069541 PR Review Comment: https://git.openjdk.org/jdk/pull/26155#discussion_r2242070753 PR Review Comment: https://git.openjdk.org/jdk/pull/26155#discussion_r2242070041 PR Review Comment: https://git.openjdk.org/jdk/pull/26155#discussion_r2242070310 PR Review Comment: https://git.openjdk.org/jdk/pull/26155#discussion_r2242070550 PR Review Comment: https://git.openjdk.org/jdk/pull/26155#discussion_r2242070964 PR Review Comment: https://git.openjdk.org/jdk/pull/26155#discussion_r2242071082 PR Review Comment: https://git.openjdk.org/jdk/pull/26155#discussion_r2242071466 PR Review Comment: https://git.openjdk.org/jdk/pull/26155#discussion_r2242072115
