On Thu, 5 Jun 2025 13:59:47 GMT, Jaikiran Pai <j...@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`

Good idea. FWIW, and ulness I'm mistaken, I believe the old code was not always 
throwing `FileNotFoundException` - but might have thrown `NoSuchFileException` 
if  using  a non-default filesystem. If `FileNotFoundException` is specified 
and `NoSuchFileException` is not then that's probably a good change of 
behaviour.

Could be worth a release note (if that's the case and the behaviour changed) - 
but probably not a CSR.

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

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

Reply via email to