On Thu, 5 Jun 2025 13:30:10 GMT, Volkan Yazici <vyaz...@openjdk.org> wrote:

> Simplifies content streaming for `HttpRequest.BodyPublishers::ofFile(Path)`, 
> which delegates to `RequestPublishers.FilePublisher::create(Path)`. This 
> effectively replaces all usages of `FileInputStream::new` in `HttpClient` 
> with `Files::newInputStream(Path)`. See the JBS ticket description for the 
> full story.
> 
> Also removing certain test files which were rendered redundant after 
> `SecurityManager` removal. We do this in this PR, since tests were added due 
> to touched lines.
> 
> `tier1,2` results on b5f3c8dc6f4 are attached to the ticket.

src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java 
line 269:

> 267:                 // The old code was using `FileInputStream::new`, which 
> throws `FNFE` if file doesn't exist.
> 268:                 // Preserving that behaviour after migrating to 
> `Files::newInputStream`:
> 269:                 t = new FileNotFoundException(path + " (No such file or 
> directory)");

Hello Volkan, just some initial comments. I think this 
`Files.isRegularFile(...)` check and the `Files.newInputStream(...)` call 
should be done in `RequestPublishers.create(...)` instead of here in subscribe. 
That way the checks happen when the `BodyPublishers.ofFile(...)` gets invoked 
instead of at subscription time. That then means that this `FilePublisher` will 
continue to accept a `InputStream` in its constructor, created out of a 
successful call to `Files.newInputStream()`. 

Moving these checks and call into `RequestPublishers.create(...)` will also 
mean that we won't need the code comments explaining why a 
`FileNotFoundException` gets thrown, because both `BodyPublishers.ofFile(...)` 
and the (internal) `RequestPublishers.create(...)` are already specified to 
throw `FileNotFoundException`

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/25662#discussion_r2128933747

Reply via email to