On Thu, 21 Aug 2025 09:43:47 GMT, Volkan Yazici <[email protected]> wrote:
> Improves exception handling of built-in `HttpClient.BodyPublisher`s to ensure > exceptions get propagated in a way compliant with the reactive specification. > I don't think we need to add new `throws Exception` declarations if the > existing code does not need it. @liach, I've did some cleaning in 89dba9a14cd. Let me know if you were concerned about more places. @jaikiran, created the [JDK-8367067] sub-task to dedicate this PR to the hardening of exception handling in built-in body publishers – recall that the parent issue, [JDK-8364733], of exceptions leaking for `sendAsync` still needs to be addressed. I had already removed the duplication in tests, though @dfuch suggested to do the clean-up in a follow-up ticket. Hence, 1. Removed clean-up related changes in fa1a7368fee to keep the PR focused 2. Created the [JDK-8367068] subtask for the clean-up [JDK-8364733]: https://bugs.openjdk.org/browse/JDK-8364733 [JDK-8367067]: https://bugs.openjdk.org/browse/JDK-8367067 [JDK-8367068]: https://bugs.openjdk.org/browse/JDK-8367068 src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java line 77: > 75: > 76: private ByteArrayPublisher(byte[] content, int offset, int > length, int bufSize) { > 77: Objects.checkFromIndexSize(offset, length, content.length); > // Implicit null check on `content` This check already exists in the `HttpRequest.BodyPublishers#ofByteArray(byte[],int,int)` public API – though duplicating it here since this is the canonical constructor. src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java line 78: > 76: private ByteArrayPublisher(byte[] content, int offset, int > length, int bufSize) { > 77: Objects.checkFromIndexSize(offset, length, content.length); > // Implicit null check on `content` > 78: if (bufSize <= 0) { New validation to checking on `bufSize`. src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java line 342: > 340: is.close(); > 341: } catch (IOException e) { > 342: throw new UncheckedIOException(e); We don't convert `IOE`s to `UncheckedIOE`s anymore. This effectively makes the `HttpClientImpl::send` exception translation logic branching on `if (throwable instanceof IOException)` to kick in. src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java line 357: > 355: haveNext = false; > 356: need2Read = false; > 357: throw new IOException(e); We don't convert `IOE`s to `UncheckedIOE`s anymore. This effectively makes the `HttpClientImpl::send` exception translation logic branching on `if (throwable instanceof IOException)` to kick in. src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java line 395: > 393: Exception exception = null; > 394: try { > 395: is = streamSupplier.get(); Fixes leaked `streamSupplier.get()` failures src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java line 486: > 484: } catch (IOException ioe) { > 485: terminated = true; > 486: throw new IOException(ioe); We don't convert `IOE`s to `UncheckedIOE`s anymore. This effectively makes the `HttpClientImpl::send` exception translation logic branching on `if (throwable instanceof IOException)` to kick in. ------------- PR Review: https://git.openjdk.org/jdk/pull/26876#pullrequestreview-3143702796 PR Comment: https://git.openjdk.org/jdk/pull/26876#issuecomment-3265349702 PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2329446658 PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2329449047 PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2329474662 PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2329474192 PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2329441592 PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2329475172
