On Tue, 19 Aug 2025 06:40:18 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Volkan Yazici has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Improve style for declaring multiple consecutive fields of the same type
>
> The implementation and test changes look good to me.
>
> I had some suggestions for the javadoc text. Here's what I had in mind, if
> Daniel and Michael suggest that the current form in this PR is OK, then it's
> OK to not change it:
>
>
> /**
> * {@return a request body publisher whose body is the {@code length}
> * content bytes read from the provided file {@code channel} starting
> * from the specified {@code offset}}
> * <p>
> * This method does not modify the {@code channel}'s position.
> * <p>
> * This method does not close the {@code channel}. The caller is
> * expected to close the {@code channel} when no longer needed.
> *
> * @apiNote
> * This method can be used to either publish just a portion of a
> * file's content as the request body or to publish different portions
> * of the file's content concurrently.
> * The typical approach to concurrently publish different portions of a
> * file's content is to create an instance of {@link FileChannel}
> * and then create multiple {@code HttpRequest}s each of which use
> * a {@code ofFileChannel BodyPublisher} with a different non-overlapping
> * offset and length.
> *
> * @param channel a file channel
> * @param offset the offset of the first byte
> * @param length the number of bytes to read from the file channel
> *
> * @throws IndexOutOfBoundsException if the specified byte range is
> * found to be {@linkplain Objects#checkFromIndexSize(long, long, long)
> * out of bounds} compared with the size of the file referred by the
> * channel
> *
> * @throws IOException if the size of the {@code channel} cannot be
> * determined or the {@code channel} is closed
> *
> * @throws NullPointerException if {@code channel} is null
> *
> * @since 26
> */
>
>
>
> (Apart from the text changes, do note that the `@throws IOException`
> specification in that text is intentionally changed to note that it will be
> thrown if the given channel is closed, in addition to if the size() cannot be
> determined).
@jaikiran, regarding your spec. suggestion:
* content bytes read from the provided file {@code channel} starting
* from the specified {@code offset}}
* <p>
- * The {@linkplain FileChannel file channel} will be read using
- * {@link FileChannel#read(ByteBuffer, long)
FileChannel.read(ByteBuffer buffer, long position)},
- * which does not modify the channel's position. Thus, the same file
- * channel may be shared between several publishers passed to
- * concurrent requests.
+ * This method does not modify the {@code channel}'s position.
* <p>
- * The file channel will not be closed upon completion. The caller is
- * expected to manage the life cycle of the channel, and close it
- * appropriately when not needed anymore.
+ * This method does not close the {@code channel}. The caller is
+ * expected to close the {@code channel} when no longer needed.
+ *
+ * @apiNote
+ * This method can be used to either publish just a portion of a
+ * file's content as the request body or to publish different portions
+ * of the file's content concurrently.
+ * The typical approach to concurrently publish different portions of a
+ * file's content is to create an instance of {@link FileChannel}
+ * and then create multiple {@code HttpRequest}s each of which use
+ * a {@code ofFileChannel BodyPublisher} with a different
non-overlapping
+ * offset and length.
*
* @param channel a file channel
* @param offset the offset of the first byte
@@ -744,8 +749,8 @@
* out of bounds} compared with the size of the file referred by the
* channel
*
- * @throws IOException if the size of the file referred by the provided
- * channel cannot be read while verifying the specified byte range
+ * @throws IOException if the size of the {@code channel} cannot be
+ * determined or the {@code channel} is closed
*
* @throws NullPointerException if {@code channel} is null
*
1. I see you removed the explicit mention of the `FileChannel#read(ByteBuffer,
long)` usage. I had placed it intentionally there, as requested by @dfuch, for
`FileChannel` can have custom implementations, and we better communicate what
`FC` method we use under the hood and with which assumptions.
2. Regarding the _"the size of the {@code channel}"_ expression, maybe a little
bit grammar policing, but a channel doesn't have a size, instead, the file
referred by the channel has a size – `FC::size` Javadoc is worded in this way
too.
I will wait for the input of @dfuch and/or @Michael-Mc-Mahon, and then submit
the CSR.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/26155#issuecomment-3199748167
