You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "ppkarwasz (via GitHub)" <gi...@apache.org> on 2025/09/21 16:11:33 UTC
[PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
ppkarwasz opened a new pull request, #784:
URL: https://github.com/apache/commons-io/pull/784
Some builders derived from `AbstractStreamBuilder` (for example, `SevenZFile.Builder` in Commons Compress) need to produce a `SeekableByteChannel` as their data source. Until now this required ad-hoc `instanceof` switches across different origin types.
This change integrates channel support directly into the origin/builder abstraction, leading to a cleaner and more object-oriented design.
### Key changes
* Add `getReadableByteChannel()` and `getWritableByteChannel()` to `AbstractOrigin` and propagate to `AbstractStreamBuilder`.
* Introduce `ChannelOrigin`, an `AbstractOrigin` implementation backed by an existing `ReadableByteChannel`/`WritableByteChannel`.
* Add `ByteArrayChannel`, a simple in-memory `SeekableByteChannel` implementation.
* Extend unit tests to cover the new methods and types.
Before you push a pull request, review this list:
- [x] Read the [contribution guidelines](CONTRIBUTING.md) for this project.
- [x] Read the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) if you use Artificial Intelligence (AI).
- [x] I used AI to improve the PR description.
- [x] Run a successful build using the default [Maven](https://maven.apache.org/) goal with `mvn`; that's `mvn` on the command line by itself.
- [x] Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best-practice.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #784:
URL: https://github.com/apache/commons-io/pull/784#issuecomment-3316288913
> Hi @garydgregory,
>
> I noticed you just added [`ArraySeekableByteChannel`](https://github.com/apache/commons-io/blob/master/src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java) from Commons Compress into IO. I was considering doing the same, but ended up writing a fresh implementation because the original class doesn’t fully meet the NIO channel concurrency contract:
>
> * [`Channel#close()`](https://docs.oracle.com/javase/8/docs/api/java/nio/channels/Channel.html#close--) calls must be serialized, and the closed state must be immediately visible to other threads. This is correctly handled in `ByteArraySeekableByteChannel`.
> * [`ReadableByteChannel#read(...)`](https://docs.oracle.com/javase/8/docs/api/java/nio/channels/ReadableByteChannel.html#read-java.nio.ByteBuffer-) invocations must be serialized: BASBC does not enforce this.
> * [`WritableByteChannel#write(...)`](https://docs.oracle.com/javase/8/docs/api/java/nio/channels/WritableByteChannel.html#write-java.nio.ByteBuffer-) invocations must also be serialized: BASBC doesn’t enforce this either.
Hi @ppkarwasz
Yes, you're right of course 😄
Please see git master for the classic reentrant lock approach.
Would you mind providing any additional modifications to `ByteArraySeekableByteChannel` either here or separately?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2368195457
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -137,6 +372,16 @@ public Writer getWriter(final Charset charset, final OpenOption... options) thro
return new OutputStreamWriter(getOutputStream(options), Charsets.toCharset(charset));
}
+ @Override
+ public ReadableByteChannel getReadableByteChannel(OpenOption... options) throws IOException {
+ return getRandomAccessFile(options).getChannel();
Review Comment:
I had considered such a hierarchy but initially rejected it for the simplicity of a "flat" hierarchy, which is no longer true since the introduction of `AbstractRandomAccessFileOrigin`. It might be time to revisit this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2378961467
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -755,6 +1120,28 @@ public Writer getWriter(final Charset charset, final OpenOption... options) thro
return Files.newBufferedWriter(getPath(), Charsets.toCharset(charset), options);
}
+ /**
+ * Gets this origin as a Channel, if possible.
+ *
+ * @param channelType The type of channel to return.
+ * @param options Options specifying how a file-based origin is opened, ignored otherwise.
+ * @return A new Channel on the origin of the given type.
+ * @param <C> The type of channel to return.
+ * @throws IOException If an I/O error occurs.
+ * @throws UnsupportedOperationException If this origin cannot be converted to a channel of the given type.
+ * @since 2.21.0
+ */
+ public <C extends Channel> C getChannel(Class<C> channelType, OpenOption... options) throws IOException {
+ Objects.requireNonNull(channelType, "channelType");
+ final Channel channel = doGetChannel(options);
+ if (channelType.isInstance(channel)) {
+ return channelType.cast(channel);
+ }
+ throw unsupportedChannelType(channelType);
+ }
+
+ abstract Channel doGetChannel(OpenOption... options) throws IOException;
Review Comment:
Done in https://github.com/apache/commons-io/pull/784/commits/c75480d9e703cc8d45228b3f4e43c5dd16fc6202
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2366838399
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -203,7 +452,7 @@ public CharSequenceOrigin(final CharSequence origin) {
@Override
public byte[] getByteArray() {
// TODO Pass in a Charset? Consider if call sites actually need this.
- return origin.toString().getBytes(Charset.defaultCharset());
+ return getCharSequence(null).toString().getBytes(Charset.defaultCharset());
Review Comment:
Fixed in https://github.com/apache/commons-io/pull/784/commits/24fff74dc1645e7b566fb167befe1ad167c1ba62
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2371451234
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,74 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
*/
public class ByteArraySeekableByteChannel implements SeekableByteChannel {
private static final int RESIZE_LIMIT = Integer.MAX_VALUE >> 1;
+
+ /**
+ * Constructs a new channel backed directly by the given byte array.
+ *
+ * <p>The channel initially contains the full contents of the array, with its
+ * size set to {@code bytes.length} and its position set to {@code 0}.</p>
+ *
+ * <p>Reads and writes operate on the shared array.
+ * If a write operation extends beyond the current capacity, the channel will
+ * automatically allocate a larger backing array and copy the existing contents.</p>
+ *
+ * @param bytes The byte array to wrap, must not be {@code null}
+ * @return A new channel that uses the given array as its initial backing store
+ * @throws NullPointerException If {@code bytes} is {@code null}
+ * @see #array()
+ * @see java.io.ByteArrayInputStream#ByteArrayInputStream(byte[])
+ */
+ public static ByteArraySeekableByteChannel wrap(byte[] bytes) {
+ Objects.requireNonNull(bytes, "bytes");
+ return new ByteArraySeekableByteChannel(bytes);
+ }
+
private byte[] data;
- private final AtomicBoolean closed = new AtomicBoolean();
+ private volatile boolean closed;
private int position;
private int size;
private final ReentrantLock lock = new ReentrantLock();
/**
- * Constructs a new instance using a default empty buffer.
+ * Constructs a new instance, with a default internal buffer capacity.
+ * <p>
+ * The initial size and position of the channel are 0.
+ * </p>
+ * <p>
+ * The default initial capacity is 32 bytes, although the capacity will grow as needed when writing data.
+ * </p>
+ * @see java.io.ByteArrayOutputStream#ByteArrayOutputStream()
Review Comment:
Fixed in https://github.com/apache/commons-io/pull/784/commits/b2ac652df0f24614995e48fa7d6ba236149aa633
To remove the FQCN I had to add an import.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #784:
URL: https://github.com/apache/commons-io/pull/784#issuecomment-3340168318
@ppkarwasz
PR merged. TY!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #784:
URL: https://github.com/apache/commons-io/pull/784
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2368015636
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,66 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
*/
public class ByteArraySeekableByteChannel implements SeekableByteChannel {
private static final int RESIZE_LIMIT = Integer.MAX_VALUE >> 1;
+
+ /**
+ * Constructs a channel that wraps the given byte array.
+ * <p>
+ * The resulting channel will share the given array as its buffer, until a write operation
+ * requires a larger capacity.
+ * The initial size of the channel is the length of the given array, and the initial position is 0.
+ * </p>
+ * @param bytes The byte array to wrap; must not be {@code null}.
+ * @return A new channel that wraps the given byte array; never {@code null}.
+ * @throws NullPointerException If the byte array is {@code null}.
+ */
+ public static ByteArraySeekableByteChannel wrap(byte[] bytes) {
+ Objects.requireNonNull(bytes, "bytes");
+ return new ByteArraySeekableByteChannel(bytes, bytes.length);
+ }
+
private byte[] data;
- private final AtomicBoolean closed = new AtomicBoolean();
+ private volatile boolean closed;
private int position;
private int size;
private final ReentrantLock lock = new ReentrantLock();
/**
* Constructs a new instance using a default empty buffer.
+ * <p>
+ * The initial size of the channel is 0, and the initial position is 0.
+ * </p>
*/
public ByteArraySeekableByteChannel() {
this(0);
}
- /**
- * Constructs a new instance from a byte array.
- *
- * @param data input data or pre-allocated array.
- */
- public ByteArraySeekableByteChannel(final byte[] data) {
- this.data = data;
- this.size = data.length;
- }
-
/**
* Constructs a new instance from a size of storage to be allocated.
- *
+ * <p>
+ * The initial size of the channel is 0, and the initial position is 0.
+ * </p>
* @param size size of internal buffer to allocate, in bytes.
*/
public ByteArraySeekableByteChannel(final int size) {
- this(new byte[size]);
+ this(byteArray(size), 0);
+ }
+
+ private static byte[] byteArray(int value) {
+ if (value < 0) {
+ throw new IllegalArgumentException("Size must be non-negative");
+ }
+ return new byte[value];
+ }
+
+ private ByteArraySeekableByteChannel(byte[] data, int size) {
Review Comment:
The lack of validation is the reason the constructor is private…
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2368889461
##########
src/main/java/org/apache/commons/io/build/AbstractOriginSupplier.java:
##########
@@ -368,4 +376,26 @@ public B setURI(final URI origin) {
public B setWriter(final Writer origin) {
return setOrigin(newWriterOrigin(origin));
}
+
+ /**
+ * Sets a new origin.
+ *
+ * @param origin the new origin.
+ * @return {@code this} instance.
+ * @since 2.21.0
+ */
+ public B setReadableByteChannel(final ReadableByteChannel origin) {
+ return setOrigin(newChannelOrigin(origin));
+ }
+
+ /**
+ * Sets a new origin.
+ *
+ * @param origin the new origin.
+ * @return {@code this} instance.
+ * @since 2.21.0
+ */
+ public B setWritableByteChannel(final WritableByteChannel origin) {
Review Comment:
You make a good point. Since there are quite a few `Channel` sub-interfaces, how about introducing a single generic accessor on `AbstractOrigin` instead of separate getters? For example:
```java
public <T extends Channel> T getChannel(Class<T> type);
```
Each implementation could then attempt to provide the requested type or fail fast if it can’t.
On the builder side we can keep it simple with a single setter:
```java
public B setChannel(Channel channel);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2366298364
##########
src/main/java/org/apache/commons/io/channels/ByteArrayChannel.java:
##########
@@ -0,0 +1,241 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.io.channels;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.channels.ClosedChannelException;
+import java.nio.channels.SeekableByteChannel;
+import java.util.Arrays;
+import java.util.Objects;
+import java.util.concurrent.locks.ReentrantLock;
+
+import org.apache.commons.io.IOUtils;
+
+/**
+ * An in-memory {@link SeekableByteChannel} backed by a growable {@code byte[]} buffer.
+ *
+ * @since 2.21.0
+ */
+public class ByteArrayChannel implements SeekableByteChannel {
Review Comment:
Hello @ppkarwasz
This appears to duplicate the new ByteArraySeekableByteChannel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2371454105
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,74 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
*/
public class ByteArraySeekableByteChannel implements SeekableByteChannel {
private static final int RESIZE_LIMIT = Integer.MAX_VALUE >> 1;
+
+ /**
+ * Constructs a new channel backed directly by the given byte array.
+ *
+ * <p>The channel initially contains the full contents of the array, with its
+ * size set to {@code bytes.length} and its position set to {@code 0}.</p>
+ *
+ * <p>Reads and writes operate on the shared array.
+ * If a write operation extends beyond the current capacity, the channel will
+ * automatically allocate a larger backing array and copy the existing contents.</p>
+ *
+ * @param bytes The byte array to wrap, must not be {@code null}
+ * @return A new channel that uses the given array as its initial backing store
+ * @throws NullPointerException If {@code bytes} is {@code null}
+ * @see #array()
+ * @see java.io.ByteArrayInputStream#ByteArrayInputStream(byte[])
+ */
+ public static ByteArraySeekableByteChannel wrap(byte[] bytes) {
+ Objects.requireNonNull(bytes, "bytes");
+ return new ByteArraySeekableByteChannel(bytes);
+ }
+
private byte[] data;
- private final AtomicBoolean closed = new AtomicBoolean();
+ private volatile boolean closed;
private int position;
private int size;
private final ReentrantLock lock = new ReentrantLock();
/**
- * Constructs a new instance using a default empty buffer.
+ * Constructs a new instance, with a default internal buffer capacity.
+ * <p>
+ * The initial size and position of the channel are 0.
+ * </p>
+ * <p>
+ * The default initial capacity is 32 bytes, although the capacity will grow as needed when writing data.
+ * </p>
+ * @see java.io.ByteArrayOutputStream#ByteArrayOutputStream()
*/
public ByteArraySeekableByteChannel() {
- this(0);
+ this(32);
}
/**
- * Constructs a new instance from a byte array.
- *
- * @param data input data or pre-allocated array.
+ * Constructs a new instance, with an internal buffer of the given capacity, in bytes.
+ * <p>
+ * The initial size and position of the channel are 0.
+ * </p>
+ * @param size Capacity of the internal buffer to allocate, in bytes.
+ * @see java.io.ByteArrayOutputStream#ByteArrayOutputStream(int)
Review Comment:
Fixed in https://github.com/apache/commons-io/pull/784/commits/b2ac652df0f24614995e48fa7d6ba236149aa633
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2371504289
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -53,15 +58,246 @@
import org.apache.commons.io.output.WriterOutputStream;
/**
- * Abstracts the origin of data for builders like a {@link File}, {@link Path}, {@link Reader}, {@link Writer}, {@link InputStream}, {@link OutputStream}, and
- * {@link URI}.
+ * Abstract base class that encapsulates the <em>origin</em> of data used by Commons IO builders.
* <p>
- * Some methods may throw {@link UnsupportedOperationException} if that method is not implemented in a concrete subclass, see {@link #getFile()} and
- * {@link #getPath()}.
+ * An origin represents where bytes/characters come from or go to, such as a {@link File}, {@link Path},
+ * {@link Reader}, {@link Writer}, {@link InputStream}, {@link OutputStream}, or {@link URI}. Concrete subclasses
+ * expose only the operations that make sense for the underlying source or sink; invoking an unsupported operation
+ * results in {@link UnsupportedOperationException} (see, for example, {@link #getFile()} and {@link #getPath()}).
* </p>
*
- * @param <T> the type of instances to build.
- * @param <B> the type of builder subclass.
+ * <p>
+ * The table below summarizes which views and conversions are supported for each origin type.
+ * Column headers show the target view; cells indicate whether that view is available from the origin in that row.
+ * </p>
+ *
+ * <table>
+ * <caption>Origin support matrix</caption>
Review Comment:
There should be entries for both `ReadableByteChannel` and `WritableByteChannel`. The table contains readable resources first (`InputStream`, `Reader`, RBC) and writable resources next (`OutputStream`, `Writer`, WBC).
Initially I also added `SeekableByteChannel`, but it seemed confusing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #784:
URL: https://github.com/apache/commons-io/pull/784#issuecomment-3321187547
@ppkarwasz
> I’m not strongly against exposing a public `ByteArraySeekableByteChannel(byte[])` constructor, though I personally prefer the `wrap` factory method.
Would you please explain why one versus the other?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #784:
URL: https://github.com/apache/commons-io/pull/784#issuecomment-3334573122
FYI https://www.merriam-webster.com/dictionary/feat
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #784:
URL: https://github.com/apache/commons-io/pull/784#issuecomment-3318857141
> @garydgregory
>
> Just a heads-up: in [474f5e8](https://github.com/apache/commons-io/commit/474f5e8cc80bf8557d87d48ef512b5a983832ccd) I refactored the `ByteArraySeekableByteChannel` constructors and factory methods to align more closely with the `ByteArrayInputStream`/`ByteArrayOutputStream` patterns:
>
> * Default buffer size is now 32, matching `ByteArrayOutputStream`.
> * Javadoc now cross-references the corresponding BAIS/BAOS constructors.
>
> I’m not strongly against exposing a public `ByteArraySeekableByteChannel(byte[])` constructor, though I personally prefer the `wrap` factory method.
Most if not all buffers in Commons IO are `IOUtils.DEFAULT_BUFFER_SIZE` (8K), attempting to align with OS file block sizes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2370318884
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -755,6 +1120,28 @@ public Writer getWriter(final Charset charset, final OpenOption... options) thro
return Files.newBufferedWriter(getPath(), Charsets.toCharset(charset), options);
}
+ /**
+ * Gets this origin as a Channel, if possible.
+ *
+ * @param channelType The type of channel to return.
+ * @param options Options specifying how a file-based origin is opened, ignored otherwise.
+ * @return A new Channel on the origin of the given type.
+ * @param <C> The type of channel to return.
+ * @throws IOException If an I/O error occurs.
+ * @throws UnsupportedOperationException If this origin cannot be converted to a channel of the given type.
+ * @since 2.21.0
+ */
+ public <C extends Channel> C getChannel(Class<C> channelType, OpenOption... options) throws IOException {
+ Objects.requireNonNull(channelType, "channelType");
+ final Channel channel = doGetChannel(options);
+ if (channelType.isInstance(channel)) {
+ return channelType.cast(channel);
+ }
+ throw unsupportedChannelType(channelType);
+ }
+
+ abstract Channel doGetChannel(OpenOption... options) throws IOException;
Review Comment:
What is this supposed to do? A Javadoc would help 😉
I'm _really_ not a fan of the `doFoo` naming convention. To me it screams: "I couldn't think of a better name!". It looks like all of these implementations create a _new_ channel (except `ChannelOrigin` obviously), so... how about calling it`newChannel()`? At least that name is pretty clear, even without a Javadoc 😉
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,74 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
*/
public class ByteArraySeekableByteChannel implements SeekableByteChannel {
private static final int RESIZE_LIMIT = Integer.MAX_VALUE >> 1;
+
+ /**
+ * Constructs a new channel backed directly by the given byte array.
+ *
+ * <p>The channel initially contains the full contents of the array, with its
+ * size set to {@code bytes.length} and its position set to {@code 0}.</p>
+ *
+ * <p>Reads and writes operate on the shared array.
+ * If a write operation extends beyond the current capacity, the channel will
+ * automatically allocate a larger backing array and copy the existing contents.</p>
+ *
+ * @param bytes The byte array to wrap, must not be {@code null}
+ * @return A new channel that uses the given array as its initial backing store
+ * @throws NullPointerException If {@code bytes} is {@code null}
+ * @see #array()
+ * @see java.io.ByteArrayInputStream#ByteArrayInputStream(byte[])
+ */
+ public static ByteArraySeekableByteChannel wrap(byte[] bytes) {
+ Objects.requireNonNull(bytes, "bytes");
+ return new ByteArraySeekableByteChannel(bytes);
+ }
+
private byte[] data;
- private final AtomicBoolean closed = new AtomicBoolean();
+ private volatile boolean closed;
private int position;
private int size;
private final ReentrantLock lock = new ReentrantLock();
/**
- * Constructs a new instance using a default empty buffer.
+ * Constructs a new instance, with a default internal buffer capacity.
+ * <p>
+ * The initial size and position of the channel are 0.
+ * </p>
+ * <p>
+ * The default initial capacity is 32 bytes, although the capacity will grow as needed when writing data.
+ * </p>
+ * @see java.io.ByteArrayOutputStream#ByteArrayOutputStream()
Review Comment:
There should be a blank Javadoc line before the first `@` tag.
Don't use FQCN when you don't need to in `@see java.io...`
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -53,15 +58,246 @@
import org.apache.commons.io.output.WriterOutputStream;
/**
- * Abstracts the origin of data for builders like a {@link File}, {@link Path}, {@link Reader}, {@link Writer}, {@link InputStream}, {@link OutputStream}, and
- * {@link URI}.
+ * Abstract base class that encapsulates the <em>origin</em> of data used by Commons IO builders.
* <p>
- * Some methods may throw {@link UnsupportedOperationException} if that method is not implemented in a concrete subclass, see {@link #getFile()} and
- * {@link #getPath()}.
+ * An origin represents where bytes/characters come from or go to, such as a {@link File}, {@link Path},
+ * {@link Reader}, {@link Writer}, {@link InputStream}, {@link OutputStream}, or {@link URI}. Concrete subclasses
+ * expose only the operations that make sense for the underlying source or sink; invoking an unsupported operation
+ * results in {@link UnsupportedOperationException} (see, for example, {@link #getFile()} and {@link #getPath()}).
* </p>
*
- * @param <T> the type of instances to build.
- * @param <B> the type of builder subclass.
+ * <p>
+ * The table below summarizes which views and conversions are supported for each origin type.
+ * Column headers show the target view; cells indicate whether that view is available from the origin in that row.
+ * </p>
+ *
+ * <table>
+ * <caption>Origin support matrix</caption>
Review Comment:
Nice! I see entries for `WritableByteChannel`, but shouldn't there be entries for `ReadableByteChannel`, and `SeekableByteChannel`?
Or should there be a single entry for `Channel`?
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,74 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
Review Comment:
Good catch! 👍
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,74 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
*/
public class ByteArraySeekableByteChannel implements SeekableByteChannel {
private static final int RESIZE_LIMIT = Integer.MAX_VALUE >> 1;
+
+ /**
+ * Constructs a new channel backed directly by the given byte array.
+ *
+ * <p>The channel initially contains the full contents of the array, with its
+ * size set to {@code bytes.length} and its position set to {@code 0}.</p>
+ *
+ * <p>Reads and writes operate on the shared array.
+ * If a write operation extends beyond the current capacity, the channel will
+ * automatically allocate a larger backing array and copy the existing contents.</p>
+ *
+ * @param bytes The byte array to wrap, must not be {@code null}
+ * @return A new channel that uses the given array as its initial backing store
+ * @throws NullPointerException If {@code bytes} is {@code null}
+ * @see #array()
+ * @see java.io.ByteArrayInputStream#ByteArrayInputStream(byte[])
+ */
+ public static ByteArraySeekableByteChannel wrap(byte[] bytes) {
+ Objects.requireNonNull(bytes, "bytes");
+ return new ByteArraySeekableByteChannel(bytes);
+ }
+
private byte[] data;
- private final AtomicBoolean closed = new AtomicBoolean();
+ private volatile boolean closed;
private int position;
private int size;
private final ReentrantLock lock = new ReentrantLock();
/**
- * Constructs a new instance using a default empty buffer.
+ * Constructs a new instance, with a default internal buffer capacity.
+ * <p>
+ * The initial size and position of the channel are 0.
+ * </p>
+ * <p>
+ * The default initial capacity is 32 bytes, although the capacity will grow as needed when writing data.
+ * </p>
+ * @see java.io.ByteArrayOutputStream#ByteArrayOutputStream()
*/
public ByteArraySeekableByteChannel() {
- this(0);
+ this(32);
}
/**
- * Constructs a new instance from a byte array.
- *
- * @param data input data or pre-allocated array.
+ * Constructs a new instance, with an internal buffer of the given capacity, in bytes.
+ * <p>
+ * The initial size and position of the channel are 0.
+ * </p>
+ * @param size Capacity of the internal buffer to allocate, in bytes.
+ * @see java.io.ByteArrayOutputStream#ByteArrayOutputStream(int)
Review Comment:
Don't use FQCN when you don't need to in `@see java.io...`
##########
src/main/java/org/apache/commons/io/build/AbstractStreamBuilder.java:
##########
@@ -258,6 +260,23 @@ public Writer getWriter() throws IOException {
return checkOrigin().getWriter(getCharset(), getOpenOptions());
}
+ /**
+ * Gets a Channel from the origin with OpenOption[].
+ *
+ * @param channelType The channel type, not null.
+ * @return A channel of the specified type.
+ * @param <C> The channel type.
+ * @throws IllegalStateException if the {@code origin} is {@code null}.
+ * @throws UnsupportedOperationException if the origin cannot be converted to a {@link ReadableByteChannel}.
+ * @throws IOException if an I/O error occurs.
+ * @see AbstractOrigin#getChannel
+ * @see #getOpenOptions()
+ * @since 2.21.0
+ */
+ public <C extends Channel> C getChannel(Class<C> channelType) throws IOException {
Review Comment:
Yeah, that's likely a good general solution 👍
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,74 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
*/
public class ByteArraySeekableByteChannel implements SeekableByteChannel {
private static final int RESIZE_LIMIT = Integer.MAX_VALUE >> 1;
+
+ /**
+ * Constructs a new channel backed directly by the given byte array.
+ *
+ * <p>The channel initially contains the full contents of the array, with its
+ * size set to {@code bytes.length} and its position set to {@code 0}.</p>
+ *
+ * <p>Reads and writes operate on the shared array.
+ * If a write operation extends beyond the current capacity, the channel will
+ * automatically allocate a larger backing array and copy the existing contents.</p>
+ *
+ * @param bytes The byte array to wrap, must not be {@code null}
+ * @return A new channel that uses the given array as its initial backing store
+ * @throws NullPointerException If {@code bytes} is {@code null}
+ * @see #array()
+ * @see java.io.ByteArrayInputStream#ByteArrayInputStream(byte[])
+ */
+ public static ByteArraySeekableByteChannel wrap(byte[] bytes) {
+ Objects.requireNonNull(bytes, "bytes");
+ return new ByteArraySeekableByteChannel(bytes);
+ }
+
private byte[] data;
- private final AtomicBoolean closed = new AtomicBoolean();
+ private volatile boolean closed;
private int position;
private int size;
private final ReentrantLock lock = new ReentrantLock();
/**
- * Constructs a new instance using a default empty buffer.
+ * Constructs a new instance, with a default internal buffer capacity.
+ * <p>
+ * The initial size and position of the channel are 0.
+ * </p>
+ * <p>
+ * The default initial capacity is 32 bytes, although the capacity will grow as needed when writing data.
+ * </p>
+ * @see java.io.ByteArrayOutputStream#ByteArrayOutputStream()
*/
public ByteArraySeekableByteChannel() {
- this(0);
+ this(32);
Review Comment:
Hm, why not `IOUtils.DEFAULT_BUFFER_SIZE` (8 KiB)? Confusingly we use `1024` in our own `AbstractByteArrayOutputStream` and `ByteArrayOutputStream`.
I'm surprised this change doesn't break anything, because if you only make that change, `ByteArraySeekableByteChannelTest` breaks.
When I tried this kinf of change here and in Compress' `SeekableInMemoryByteChannel`, the tests broke when I changed the default size to `IOUtils.DEFAULT_BUFFER_SIZE`. But now it works in `SeekableInMemoryByteChannel`...
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,74 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
*/
public class ByteArraySeekableByteChannel implements SeekableByteChannel {
private static final int RESIZE_LIMIT = Integer.MAX_VALUE >> 1;
+
+ /**
+ * Constructs a new channel backed directly by the given byte array.
+ *
+ * <p>The channel initially contains the full contents of the array, with its
+ * size set to {@code bytes.length} and its position set to {@code 0}.</p>
+ *
+ * <p>Reads and writes operate on the shared array.
+ * If a write operation extends beyond the current capacity, the channel will
+ * automatically allocate a larger backing array and copy the existing contents.</p>
+ *
+ * @param bytes The byte array to wrap, must not be {@code null}
+ * @return A new channel that uses the given array as its initial backing store
+ * @throws NullPointerException If {@code bytes} is {@code null}
+ * @see #array()
+ * @see java.io.ByteArrayInputStream#ByteArrayInputStream(byte[])
+ */
+ public static ByteArraySeekableByteChannel wrap(byte[] bytes) {
+ Objects.requireNonNull(bytes, "bytes");
+ return new ByteArraySeekableByteChannel(bytes);
+ }
+
private byte[] data;
- private final AtomicBoolean closed = new AtomicBoolean();
+ private volatile boolean closed;
Review Comment:
This seems unrelated. Why is this better? Just because it's simpler? Just curious.
##########
src/test/java/org/apache/commons/io/channels/ByteArraySeekableByteChannelCompressTest.java:
##########
@@ -62,7 +66,7 @@ void testCloseIsIdempotent() throws Exception {
@ParameterizedTest
@ValueSource(ints = { 0, 1, 2, 3, 4, 5, 6 })
void testReadingFromAPositionAfterEndReturnsEOF(final int size) throws Exception {
- try (SeekableByteChannel c = new ByteArraySeekableByteChannel(size)) {
+ try (SeekableByteChannel c = ByteArraySeekableByteChannel.wrap(new byte[size])) {
Review Comment:
I don't think this is a good idea for reading tests. You want to test reading from the same array over and over when you are reading because you want to make sure there are no side-effects that incorrectly _write_ to the underlying byte array.
##########
src/test/java/org/apache/commons/io/channels/ByteArraySeekableByteChannelCompressTest.java:
##########
@@ -42,6 +42,10 @@ class ByteArraySeekableByteChannelCompressTest {
private final byte[] testData = "Some data".getBytes(StandardCharsets.UTF_8);
+ private byte[] getTestData() {
Review Comment:
This would be better named `cloneTestData` IMO to make clear to tests what kind of data we are dealing with.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2371456387
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,74 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
*/
public class ByteArraySeekableByteChannel implements SeekableByteChannel {
private static final int RESIZE_LIMIT = Integer.MAX_VALUE >> 1;
+
+ /**
+ * Constructs a new channel backed directly by the given byte array.
+ *
+ * <p>The channel initially contains the full contents of the array, with its
+ * size set to {@code bytes.length} and its position set to {@code 0}.</p>
+ *
+ * <p>Reads and writes operate on the shared array.
+ * If a write operation extends beyond the current capacity, the channel will
+ * automatically allocate a larger backing array and copy the existing contents.</p>
+ *
+ * @param bytes The byte array to wrap, must not be {@code null}
+ * @return A new channel that uses the given array as its initial backing store
+ * @throws NullPointerException If {@code bytes} is {@code null}
+ * @see #array()
+ * @see java.io.ByteArrayInputStream#ByteArrayInputStream(byte[])
+ */
+ public static ByteArraySeekableByteChannel wrap(byte[] bytes) {
+ Objects.requireNonNull(bytes, "bytes");
+ return new ByteArraySeekableByteChannel(bytes);
+ }
+
private byte[] data;
- private final AtomicBoolean closed = new AtomicBoolean();
+ private volatile boolean closed;
private int position;
private int size;
private final ReentrantLock lock = new ReentrantLock();
/**
- * Constructs a new instance using a default empty buffer.
+ * Constructs a new instance, with a default internal buffer capacity.
+ * <p>
+ * The initial size and position of the channel are 0.
+ * </p>
+ * <p>
+ * The default initial capacity is 32 bytes, although the capacity will grow as needed when writing data.
+ * </p>
+ * @see java.io.ByteArrayOutputStream#ByteArrayOutputStream()
*/
public ByteArraySeekableByteChannel() {
- this(0);
+ this(32);
Review Comment:
I restored `DEFAULT_BUFFER_SIZE` in https://github.com/apache/commons-io/pull/784/commits/94f7f783ab0790fee3d374ce6c38e398711a1f20.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2371493300
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,74 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
*/
public class ByteArraySeekableByteChannel implements SeekableByteChannel {
private static final int RESIZE_LIMIT = Integer.MAX_VALUE >> 1;
+
+ /**
+ * Constructs a new channel backed directly by the given byte array.
+ *
+ * <p>The channel initially contains the full contents of the array, with its
+ * size set to {@code bytes.length} and its position set to {@code 0}.</p>
+ *
+ * <p>Reads and writes operate on the shared array.
+ * If a write operation extends beyond the current capacity, the channel will
+ * automatically allocate a larger backing array and copy the existing contents.</p>
+ *
+ * @param bytes The byte array to wrap, must not be {@code null}
+ * @return A new channel that uses the given array as its initial backing store
+ * @throws NullPointerException If {@code bytes} is {@code null}
+ * @see #array()
+ * @see java.io.ByteArrayInputStream#ByteArrayInputStream(byte[])
+ */
+ public static ByteArraySeekableByteChannel wrap(byte[] bytes) {
+ Objects.requireNonNull(bytes, "bytes");
+ return new ByteArraySeekableByteChannel(bytes);
+ }
+
private byte[] data;
- private final AtomicBoolean closed = new AtomicBoolean();
+ private volatile boolean closed;
Review Comment:
Yes, I replaced it because it is simpler (no additional Java object) and has the exact same semantics.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2366396316
##########
src/test/java/org/apache/commons/io/build/ByteArrayOriginTest.java:
##########
@@ -82,11 +75,24 @@ void testGetRandomAccessFile(final OpenOption openOption) {
assertThrows(UnsupportedOperationException.class, () -> super.testGetRandomAccessFile(openOption));
}
+ @Override
+ @Test
+ void testGetOutputStream() {
+ // Cannot convert a byte[] to an OutputStream.
+ assertThrows(UnsupportedOperationException.class, super::testGetOutputStream);
Review Comment:
A plain `byte[]` is not a reliable backing store for an `OutputStream`.
* With `ByteArrayOutputStream` (and similar classes), the underlying array is replaced whenever it needs to grow and the original byte array will not longer receive any updates after that.
* If we were to expose an `OutputStream` directly over a fixed `byte[]`, we’d have to either throw once the array’s capacity is exceeded or silently ignore extra writes. Neither option seems broadly useful.
That’s why `ByteArrayOrigin#getOutputStream` is expected to throw.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2366356227
##########
src/changes/changes.xml:
##########
@@ -62,10 +62,12 @@ The <action> type attribute can be add,update,fix,remove.
<action dev="ggregory" type="add" due-to="Gary Gregory">Add org.apache.commons.io.output.ProxyOutputStream.writeRepeat(int, long).</action>
<action dev="pkarwasz" type="add" due-to="Piotr P. Karwasz">Add length unit support in FileSystem limits.</action>
<action dev="pkarwasz" type="add" due-to="Piotr P. Karwasz">Add IOUtils.toByteArray(InputStream, int, int) for safer chunked reading with size validation.</action>
- <action dev="pkarwasz" type="add" due-to="Piotr P. Karwasz">Add org.apache.commons.io.file.PathUtils.getPath(String, String).</action>
+ <action dev="ggregory" type="add" due-to="Gary Gregory">Add org.apache.commons.io.file.PathUtils.getPath(String, String).</action>
<action dev="ggregory" type="add" due-to="Gary Gregory">Add org.apache.commons.io.channels.ByteArraySeekableByteChannel.</action>
+ <action dev="pkarwasz" type="add" due-to="Piotr P. Karwasz">Add ByteArrayChannel, a simple in-memory `SeekableByteChannel` implementation.</action>
+ <action dev="pkarwasz" type="add" due-to="Piotr P. Karwasz">Add NIO channel support to `AbstractStreamBuilder`.</action>
<!-- UPDATE -->
- <action type="update" dev="ggregory" due-to="Gary Gregory, Dependabot">Bump org.apache.commons:commons-parent from 85 to 88 #774, #783.</action>
+ <action type="update" dev="ggregory" due-to="Gary Gregory, Dependabot">Bump org.apache.commons:commons-parent from 85 to 87 #774.</action>
Review Comment:
This change is backwards: We are on commons-parent 88 now.
##########
src/changes/changes.xml:
##########
@@ -62,10 +62,12 @@ The <action> type attribute can be add,update,fix,remove.
<action dev="ggregory" type="add" due-to="Gary Gregory">Add org.apache.commons.io.output.ProxyOutputStream.writeRepeat(int, long).</action>
<action dev="pkarwasz" type="add" due-to="Piotr P. Karwasz">Add length unit support in FileSystem limits.</action>
<action dev="pkarwasz" type="add" due-to="Piotr P. Karwasz">Add IOUtils.toByteArray(InputStream, int, int) for safer chunked reading with size validation.</action>
- <action dev="pkarwasz" type="add" due-to="Piotr P. Karwasz">Add org.apache.commons.io.file.PathUtils.getPath(String, String).</action>
+ <action dev="ggregory" type="add" due-to="Gary Gregory">Add org.apache.commons.io.file.PathUtils.getPath(String, String).</action>
<action dev="ggregory" type="add" due-to="Gary Gregory">Add org.apache.commons.io.channels.ByteArraySeekableByteChannel.</action>
+ <action dev="pkarwasz" type="add" due-to="Piotr P. Karwasz">Add ByteArrayChannel, a simple in-memory `SeekableByteChannel` implementation.</action>
+ <action dev="pkarwasz" type="add" due-to="Piotr P. Karwasz">Add NIO channel support to `AbstractStreamBuilder`.</action>
<!-- UPDATE -->
- <action type="update" dev="ggregory" due-to="Gary Gregory, Dependabot">Bump org.apache.commons:commons-parent from 85 to 88 #774, #783.</action>
+ <action type="update" dev="ggregory" due-to="Gary Gregory, Dependabot">Bump org.apache.commons:commons-parent from 85 to 87 #774.</action>
Review Comment:
This change is backward: We are currently on commons-parent 88.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2366413947
##########
src/main/java/org/apache/commons/io/build/AbstractOriginSupplier.java:
##########
@@ -368,4 +376,26 @@ public B setURI(final URI origin) {
public B setWriter(final Writer origin) {
return setOrigin(newWriterOrigin(origin));
}
+
+ /**
+ * Sets a new origin.
+ *
+ * @param origin the new origin.
+ * @return {@code this} instance.
+ * @since 2.21.0
+ */
+ public B setReadableByteChannel(final ReadableByteChannel origin) {
+ return setOrigin(newChannelOrigin(origin));
+ }
+
+ /**
+ * Sets a new origin.
+ *
+ * @param origin the new origin.
+ * @return {@code this} instance.
+ * @since 2.21.0
+ */
+ public B setWritableByteChannel(final WritableByteChannel origin) {
Review Comment:
If `newChannelOrigin(Channel)` and `ChannelOrigin.ChannelOrigin(Channel)` both take a Channel, why not simply have `setChannel(Channel)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2366902443
##########
src/main/java/org/apache/commons/io/build/AbstractOriginSupplier.java:
##########
@@ -368,4 +376,26 @@ public B setURI(final URI origin) {
public B setWriter(final Writer origin) {
return setOrigin(newWriterOrigin(origin));
}
+
+ /**
+ * Sets a new origin.
+ *
+ * @param origin the new origin.
+ * @return {@code this} instance.
+ * @since 2.21.0
+ */
+ public B setReadableByteChannel(final ReadableByteChannel origin) {
+ return setOrigin(newChannelOrigin(origin));
+ }
+
+ /**
+ * Sets a new origin.
+ *
+ * @param origin the new origin.
+ * @return {@code this} instance.
+ * @since 2.21.0
+ */
+ public B setWritableByteChannel(final WritableByteChannel origin) {
Review Comment:
I considered a single `setChannel(Channel)` as well, but decided on separate `setReadableByteChannel` and `setWritableByteChannel` methods for type safety.
While any `Channel` can be wrapped in a `ChannelOrigin`, the actual use cases differ: you need a `ReadableByteChannel` to expose an `InputStream`, and a `WritableByteChannel` to expose an `OutputStream`. Keeping the methods separate lets the compiler catch mismatches instead of deferring to runtime.
On the other hand, many channel implementations declare both interfaces but only fully support one. In those cases, errors surface **only** at runtime. Do you think it would be clearer to collapse this into a single `setChannel` method instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2371385660
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -755,6 +1120,28 @@ public Writer getWriter(final Charset charset, final OpenOption... options) thro
return Files.newBufferedWriter(getPath(), Charsets.toCharset(charset), options);
}
+ /**
+ * Gets this origin as a Channel, if possible.
+ *
+ * @param channelType The type of channel to return.
+ * @param options Options specifying how a file-based origin is opened, ignored otherwise.
+ * @return A new Channel on the origin of the given type.
+ * @param <C> The type of channel to return.
+ * @throws IOException If an I/O error occurs.
+ * @throws UnsupportedOperationException If this origin cannot be converted to a channel of the given type.
+ * @since 2.21.0
+ */
+ public <C extends Channel> C getChannel(Class<C> channelType, OpenOption... options) throws IOException {
+ Objects.requireNonNull(channelType, "channelType");
+ final Channel channel = doGetChannel(options);
+ if (channelType.isInstance(channel)) {
+ return channelType.cast(channel);
+ }
+ throw unsupportedChannelType(channelType);
+ }
+
+ abstract Channel doGetChannel(OpenOption... options) throws IOException;
Review Comment:
I renamed it to `getChannel` and changed the access to `protected` in https://github.com/apache/commons-io/pull/784/commits/b820c4497cab67fe757234b550634e89477e1974. This will allow users to reuse the `getChannel(Class<?>, OpenOption...)` implementation above.
In this case, should we make `getChannel(Class<?>, OpenOption...)` final?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2371445140
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,74 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
*/
public class ByteArraySeekableByteChannel implements SeekableByteChannel {
private static final int RESIZE_LIMIT = Integer.MAX_VALUE >> 1;
+
+ /**
+ * Constructs a new channel backed directly by the given byte array.
+ *
+ * <p>The channel initially contains the full contents of the array, with its
+ * size set to {@code bytes.length} and its position set to {@code 0}.</p>
+ *
+ * <p>Reads and writes operate on the shared array.
+ * If a write operation extends beyond the current capacity, the channel will
+ * automatically allocate a larger backing array and copy the existing contents.</p>
+ *
+ * @param bytes The byte array to wrap, must not be {@code null}
+ * @return A new channel that uses the given array as its initial backing store
+ * @throws NullPointerException If {@code bytes} is {@code null}
+ * @see #array()
+ * @see java.io.ByteArrayInputStream#ByteArrayInputStream(byte[])
+ */
+ public static ByteArraySeekableByteChannel wrap(byte[] bytes) {
+ Objects.requireNonNull(bytes, "bytes");
+ return new ByteArraySeekableByteChannel(bytes);
+ }
+
private byte[] data;
- private final AtomicBoolean closed = new AtomicBoolean();
+ private volatile boolean closed;
private int position;
private int size;
private final ReentrantLock lock = new ReentrantLock();
/**
- * Constructs a new instance using a default empty buffer.
+ * Constructs a new instance, with a default internal buffer capacity.
+ * <p>
+ * The initial size and position of the channel are 0.
+ * </p>
+ * <p>
+ * The default initial capacity is 32 bytes, although the capacity will grow as needed when writing data.
+ * </p>
+ * @see java.io.ByteArrayOutputStream#ByteArrayOutputStream()
*/
public ByteArraySeekableByteChannel() {
- this(0);
+ this(32);
Review Comment:
I changed a little bit the semantics of the `ByteArraySeekableByteChannel()` and `ByteArraySeekableByteChannel(int)` constructors: they allocate a buffer of the given size, but they keep the `size()` of the `SeekableByteChannel` equal to 0.
This way, calling `toByteArray()` right after the constructor, returns an empty array. This aligns with how `ByteArrayOutputStream#toByteArray()` works. Of course calling `array()` returns the raw backing array of the given size.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2366356227
##########
src/changes/changes.xml:
##########
@@ -62,10 +62,12 @@ The <action> type attribute can be add,update,fix,remove.
<action dev="ggregory" type="add" due-to="Gary Gregory">Add org.apache.commons.io.output.ProxyOutputStream.writeRepeat(int, long).</action>
<action dev="pkarwasz" type="add" due-to="Piotr P. Karwasz">Add length unit support in FileSystem limits.</action>
<action dev="pkarwasz" type="add" due-to="Piotr P. Karwasz">Add IOUtils.toByteArray(InputStream, int, int) for safer chunked reading with size validation.</action>
- <action dev="pkarwasz" type="add" due-to="Piotr P. Karwasz">Add org.apache.commons.io.file.PathUtils.getPath(String, String).</action>
+ <action dev="ggregory" type="add" due-to="Gary Gregory">Add org.apache.commons.io.file.PathUtils.getPath(String, String).</action>
<action dev="ggregory" type="add" due-to="Gary Gregory">Add org.apache.commons.io.channels.ByteArraySeekableByteChannel.</action>
+ <action dev="pkarwasz" type="add" due-to="Piotr P. Karwasz">Add ByteArrayChannel, a simple in-memory `SeekableByteChannel` implementation.</action>
+ <action dev="pkarwasz" type="add" due-to="Piotr P. Karwasz">Add NIO channel support to `AbstractStreamBuilder`.</action>
<!-- UPDATE -->
- <action type="update" dev="ggregory" due-to="Gary Gregory, Dependabot">Bump org.apache.commons:commons-parent from 85 to 88 #774, #783.</action>
+ <action type="update" dev="ggregory" due-to="Gary Gregory, Dependabot">Bump org.apache.commons:commons-parent from 85 to 87 #774.</action>
Review Comment:
This change is b/w: We are on commons-parent 88 now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2366352031
##########
src/test/java/org/apache/commons/io/build/ByteArrayOriginTest.java:
##########
@@ -82,11 +75,24 @@ void testGetRandomAccessFile(final OpenOption openOption) {
assertThrows(UnsupportedOperationException.class, () -> super.testGetRandomAccessFile(openOption));
}
+ @Override
+ @Test
+ void testGetOutputStream() {
+ // Cannot convert a byte[] to an OutputStream.
+ assertThrows(UnsupportedOperationException.class, super::testGetOutputStream);
Review Comment:
```java
// Cannot convert a byte[] to an OutputStream.
```
What can't a ByteArrayOutputStream be used?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #784:
URL: https://github.com/apache/commons-io/pull/784#issuecomment-3317758478
Hi @garydgregory,
I originally had a `getSeekableByteChannel()` in this PR but removed it because it felt misleading. For example, you can construct a `SeekableByteChannel` from a `FileOutputStream`, but that is obviously *not* the kind of channel Compress expects.
My current approach is to call `getReadableByteChannel()` in Compress and, if necessary, check `instanceof SeekableByteChannel`. This works in practice: see the test that covers 9 different ways of configuring a `SeekableByteChannel`:
https://github.com/apache/commons-io/blob/0c853487783a43abb85de3fcc204ecca3a5ca421/src/test/java/org/apache/commons/io/build/AbstractStreamBuilderTest.java#L119-L133
BTW, I think that `AbstractOrigin` is a bit muddled right now:
* It mixes sources and sinks. We could clean this up by splitting into two interfaces (e.g. `InputOrigin` and `OutputOrigin`), or by adding `isReadable()` / `isWritable()` markers.
* Resource ownership semantics are unclear. Some origins wrap pre-opened resources (`InputStream`, `RandomAccessFile`, `Channel`), others create new resources on demand (`byte[]`, `File`, `Path`, `URI`). Right now it’s ambiguous who “owns” the underlying resource. In practice, the builder should not take ownership unless explicitly documented; otherwise returned resources should be close-shielded.
* In Compress, builders are typically used to create an `ArchiveInputStream`, which *does* close the underlying resource when closed. That behavior should probably be made explicit in the Javadocs for `getInputStream()` and `getReadableByteChannel()`.
* The semantics of `getByteArray()` are also fuzzy: it consumes the origin, but should that also imply closing it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2366838014
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -227,7 +476,7 @@ public CharSequence getCharSequence(final Charset charset) {
@Override
public InputStream getInputStream(final OpenOption... options) throws IOException {
// TODO Pass in a Charset? Consider if call sites actually need this.
- return CharSequenceInputStream.builder().setCharSequence(getCharSequence(Charset.defaultCharset())).get();
+ return CharSequenceInputStream.builder().setCharSequence(getCharSequence(null)).get();
Review Comment:
Sorry, fixed in https://github.com/apache/commons-io/pull/784/commits/24fff74dc1645e7b566fb167befe1ad167c1ba62
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #784:
URL: https://github.com/apache/commons-io/pull/784#issuecomment-3318849154
All good topics, some comments below.
> * It mixes sources and sinks. We could clean this up by splitting into two interfaces (e.g. `InputOrigin` and `OutputOrigin`), or by adding `isReadable()` / `isWritable()` markers.
Hm, that would only work for a subset of origins. Would you leave origins like `Path`, `File`, and `SeekableByteChannel` under `AbstractOrigin`, or, create an `AbstractIoOrigin`? (I don't think we'd want to test a given file's attributes since these can change on the fly.)
I can see that you could move `InputStreamOrigin` and `ReaderOrigin` under an `AbstractInputOrigin`. Would this really buy us anything?
> Resource ownership semantics are unclear. Some origins wrap pre-opened resources (InputStream, RandomAccessFile, Channel), others create new resources on demand (byte[], File, Path, URI). Right now it’s ambiguous who “owns” the underlying resource. In practice, the builder should not take ownership unless explicitly documented; otherwise returned resources should be close-shielded.
I thought this was all clear to: The allocator owns the resources and must close it.
Are there any bugs?
> wrap pre-opened resources (InputStream, RandomAccessFile, Channel),
This falls under "the allocating call site owns the resource."
> others create new resources on demand (byte[], File, Path, URI).
There is nothing to close on these types, they are not "resources" in the traditional "implements AutoCloseable" sense. What am I missing?
> The semantics of getByteArray() are also fuzzy: it consumes the origin, but should that also imply closing it?
This falls under "the allocating call site owns the resource". This should be covered by existing unit tests. Do you see some missing or incorrect cases?
All good topics 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2371522649
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -53,15 +58,246 @@
import org.apache.commons.io.output.WriterOutputStream;
/**
- * Abstracts the origin of data for builders like a {@link File}, {@link Path}, {@link Reader}, {@link Writer}, {@link InputStream}, {@link OutputStream}, and
- * {@link URI}.
+ * Abstract base class that encapsulates the <em>origin</em> of data used by Commons IO builders.
* <p>
- * Some methods may throw {@link UnsupportedOperationException} if that method is not implemented in a concrete subclass, see {@link #getFile()} and
- * {@link #getPath()}.
+ * An origin represents where bytes/characters come from or go to, such as a {@link File}, {@link Path},
+ * {@link Reader}, {@link Writer}, {@link InputStream}, {@link OutputStream}, or {@link URI}. Concrete subclasses
+ * expose only the operations that make sense for the underlying source or sink; invoking an unsupported operation
+ * results in {@link UnsupportedOperationException} (see, for example, {@link #getFile()} and {@link #getPath()}).
* </p>
*
- * @param <T> the type of instances to build.
- * @param <B> the type of builder subclass.
+ * <p>
+ * The table below summarizes which views and conversions are supported for each origin type.
+ * Column headers show the target view; cells indicate whether that view is available from the origin in that row.
+ * </p>
+ *
+ * <table>
+ * <caption>Origin support matrix</caption>
Review Comment:
In https://github.com/apache/commons-io/pull/784/commits/ffb3780f46763c5330d61710e9d439a18e1f6f3e I added an additional column, which summarizes what kind of `Channel` one can expect from various origins.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2366393218
##########
src/main/java/org/apache/commons/io/channels/ByteArrayChannel.java:
##########
@@ -0,0 +1,241 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.io.channels;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.channels.ClosedChannelException;
+import java.nio.channels.SeekableByteChannel;
+import java.util.Arrays;
+import java.util.Objects;
+import java.util.concurrent.locks.ReentrantLock;
+
+import org.apache.commons.io.IOUtils;
+
+/**
+ * An in-memory {@link SeekableByteChannel} backed by a growable {@code byte[]} buffer.
+ *
+ * @since 2.21.0
+ */
+public class ByteArrayChannel implements SeekableByteChannel {
Review Comment:
I merged your version and mine in https://github.com/apache/commons-io/pull/784/commits/c7bfc8f978abc044e44da6cc7fa310eba01f60ef
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2366415478
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -137,6 +372,16 @@ public Writer getWriter(final Charset charset, final OpenOption... options) thro
return new OutputStreamWriter(getOutputStream(options), Charsets.toCharset(charset));
}
+ @Override
+ public ReadableByteChannel getReadableByteChannel(OpenOption... options) throws IOException {
+ return getRandomAccessFile(options).getChannel();
Review Comment:
If `RandomAccessFile.getChannel()` always returns a `FileChannel` and `FileChannel` implements
`SeekableByteChannel`, `GatheringByteChannel`, `ScatteringByteChannel`, why not just have a `getFileChannel()`? I guess that depends on the typing of the overriden method. Or maybe both of these should call a `getFileChannel()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2378962996
##########
src/test/java/org/apache/commons/io/channels/ByteArraySeekableByteChannelCompressTest.java:
##########
@@ -62,7 +66,7 @@ void testCloseIsIdempotent() throws Exception {
@ParameterizedTest
@ValueSource(ints = { 0, 1, 2, 3, 4, 5, 6 })
void testReadingFromAPositionAfterEndReturnsEOF(final int size) throws Exception {
- try (SeekableByteChannel c = new ByteArraySeekableByteChannel(size)) {
+ try (SeekableByteChannel c = ByteArraySeekableByteChannel.wrap(new byte[size])) {
Review Comment:
You are right: all but one test were not supposed to modify any data.
Fixed in https://github.com/apache/commons-io/pull/784/commits/f24ab1d326d748c4eb84e3e59249354477ddabd9
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #784:
URL: https://github.com/apache/commons-io/pull/784#issuecomment-3334645199
Hi @ppkarwasz
Would you please rebase on git master?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2369698558
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -137,6 +372,16 @@ public Writer getWriter(final Charset charset, final OpenOption... options) thro
return new OutputStreamWriter(getOutputStream(options), Charsets.toCharset(charset));
}
+ @Override
+ public ReadableByteChannel getReadableByteChannel(OpenOption... options) throws IOException {
+ return getRandomAccessFile(options).getChannel();
Review Comment:
In https://github.com/apache/commons-io/pull/784/commits/c4e3b4300f2221ccfbcb14adf5184c7be5154b8d I introduced `getChannel`, that can be used as:
```java
origin.getChannel(FileChannel.class)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #784:
URL: https://github.com/apache/commons-io/pull/784#issuecomment-3321167669
> ```java
> InputStreamOrigin
> ```
This is definitely not a bug and is the expected behavior. A couple of points:
- Maybe the type-specific origin classes like `InputStreamOrigin` shouldn't have been public, but it would be harder to provide new conversions outside the library itself, but maybe IO isn't used at that level of depth. A lesson learned for the future.
- The type-specific origin classes (here, `InputStreamOriogin`) are all _wrappers_ of that specific type, and always return the instance it was given on creation from its same-type getter (here, `getInputStream()`). In this case, an `InputStreamOrigin` is constructed with an `InputStream` and always returns that `InputStream` from `getInputStream()`
- The previous point highlights the need for better Javadocs I think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #784:
URL: https://github.com/apache/commons-io/pull/784#issuecomment-3322986322
Hi @garydgregory,
> This is definitely not a bug and is the expected behavior. A couple of points:
>
> * Maybe the type-specific origin classes like `InputStreamOrigin` shouldn't have been public, but it would be harder to provide new conversions outside the library itself, but maybe IO isn't used at that level of depth. A lesson learned for the future.
>
> * The type-specific origin classes (here, `InputStreamOriogin`) are all _wrappers_ of that specific type, and always return the instance it was given on creation from its same-type getter (here, `getInputStream()`). In this case, an `InputStreamOrigin` is constructed with an `InputStream` and always returns that `InputStream` from `getInputStream()`
>
> * The previous point highlights the need for better Javadocs I think.
Thanks for the clarification. If returning the **same instance** from type-specific origins is the intended behavior, we should make that explicit: right now callers can’t know whether `AbstractOrigin#getInputStream()` returns a fresh stream or a wrapper over an already-open resource, so they’ll (correctly) close it. So the Javadoc should say something like:
> Callers **own** the returned stream and **must** close it. Closing the returned stream closes the underlying resource. If you need a close-shielded view, wrap the stream before returning it.
Note that we should adapt the tests to also **close** the resources hold by the origin. After moving the read-write origin to a temporary per-test folder, I got some errors on Windows due to unclosed resources: https://github.com/apache/commons-io/pull/784/commits/1fc9097f3c3bd5ac3223bd91fdf265c9390a3f7b.
> > I’m not strongly against exposing a public `ByteArraySeekableByteChannel(byte[])` constructor, though I personally prefer the `wrap` factory method.
>
> Would you please explain why one versus the other?
I personally prefer a factory method (`wrap(byte[])`) over a public constructor because it allows validation, `import static` and future flexibility (e.g., caching, different implementations) without breaking API. However, in this case I don't see any possible future changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2372129902
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -755,6 +1120,28 @@ public Writer getWriter(final Charset charset, final OpenOption... options) thro
return Files.newBufferedWriter(getPath(), Charsets.toCharset(charset), options);
}
+ /**
+ * Gets this origin as a Channel, if possible.
+ *
+ * @param channelType The type of channel to return.
+ * @param options Options specifying how a file-based origin is opened, ignored otherwise.
+ * @return A new Channel on the origin of the given type.
+ * @param <C> The type of channel to return.
+ * @throws IOException If an I/O error occurs.
+ * @throws UnsupportedOperationException If this origin cannot be converted to a channel of the given type.
+ * @since 2.21.0
+ */
+ public <C extends Channel> C getChannel(Class<C> channelType, OpenOption... options) throws IOException {
+ Objects.requireNonNull(channelType, "channelType");
+ final Channel channel = doGetChannel(options);
+ if (channelType.isInstance(channel)) {
+ return channelType.cast(channel);
+ }
+ throw unsupportedChannelType(channelType);
+ }
+
+ abstract Channel doGetChannel(OpenOption... options) throws IOException;
Review Comment:
If you want, it's the more conservative choice. If we make it final now, we can always loosen it later, but not the other way around.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #784:
URL: https://github.com/apache/commons-io/pull/784#issuecomment-3316101403
Hi @garydgregory,
I noticed you just added [`ArraySeekableByteChannel`](https://github.com/apache/commons-io/blob/master/src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java) from Commons Compress into IO. I was considering doing the same, but ended up writing a fresh implementation because the original class doesn’t fully meet the NIO channel concurrency contract:
* [`Channel#close()`](https://docs.oracle.com/javase/8/docs/api/java/nio/channels/Channel.html#close--) calls must be serialized, and the closed state must be immediately visible to other threads. This is correctly handled in `ByteArraySeekableByteChannel`.
* [`ReadableByteChannel#read(...)`](https://docs.oracle.com/javase/8/docs/api/java/nio/channels/ReadableByteChannel.html#read-java.nio.ByteBuffer-) invocations must be serialized: BASBC does not enforce this.
* [`WritableByteChannel#write(...)`](https://docs.oracle.com/javase/8/docs/api/java/nio/channels/WritableByteChannel.html#write-java.nio.ByteBuffer-) invocations must also be serialized: BASBC doesn’t enforce this either.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2366838759
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -267,7 +521,7 @@ public FileOrigin(final File origin) {
@Override
public byte[] getByteArray(final long position, final int length) throws IOException {
- try (RandomAccessFile raf = RandomAccessFileMode.READ_ONLY.create(origin)) {
+ try (RandomAccessFile raf = RandomAccessFileMode.READ_ONLY.create(getFile())) {
Review Comment:
Fixed in https://github.com/apache/commons-io/pull/784/commits/24fff74dc1645e7b566fb167befe1ad167c1ba62
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #784:
URL: https://github.com/apache/commons-io/pull/784#issuecomment-3318603071
@garydgregory
Just a heads-up: in 474f5e8cc80bf8557d87d48ef512b5a983832ccd I refactored the `ByteArraySeekableByteChannel` constructors and factory methods to align more closely with the `ByteArrayInputStream`/`ByteArrayOutputStream` patterns:
* Default buffer size is now 32, matching `ByteArrayOutputStream`.
* Javadoc now cross-references the corresponding BAIS/BAOS constructors.
I’m not strongly against exposing a public `ByteArraySeekableByteChannel(byte[])` constructor, though I personally prefer the `wrap` factory method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2369693131
##########
src/main/java/org/apache/commons/io/build/AbstractOriginSupplier.java:
##########
@@ -368,4 +376,26 @@ public B setURI(final URI origin) {
public B setWriter(final Writer origin) {
return setOrigin(newWriterOrigin(origin));
}
+
+ /**
+ * Sets a new origin.
+ *
+ * @param origin the new origin.
+ * @return {@code this} instance.
+ * @since 2.21.0
+ */
+ public B setReadableByteChannel(final ReadableByteChannel origin) {
+ return setOrigin(newChannelOrigin(origin));
+ }
+
+ /**
+ * Sets a new origin.
+ *
+ * @param origin the new origin.
+ * @return {@code this} instance.
+ * @since 2.21.0
+ */
+ public B setWritableByteChannel(final WritableByteChannel origin) {
Review Comment:
In https://github.com/apache/commons-io/pull/784/commits/c4e3b4300f2221ccfbcb14adf5184c7be5154b8d I replaced the `getReadableByteChannel` and similar methods with those proposed above: the API looks much cleaner now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2372214794
##########
src/test/java/org/apache/commons/io/build/CharSequenceOriginTest.java:
##########
@@ -62,13 +62,6 @@ void testGetFile() {
assertThrows(UnsupportedOperationException.class, super::testGetFile);
}
- @Override
- @Test
- void testGetOutputStream() {
Review Comment:
Why was this method deleted and then added again? If you keep the methods in AB order, this shouln't happen.
##########
src/test/java/org/apache/commons/io/build/OutputStreamOriginTest.java:
##########
@@ -89,13 +90,6 @@ void testGetFile() {
assertThrows(UnsupportedOperationException.class, super::testGetFile);
}
- @Override
Review Comment:
This method doesn't need to be moved; it makes the PR larger for no reason.
##########
src/test/java/org/apache/commons/io/build/InputStreamOriginTest.java:
##########
@@ -58,13 +59,6 @@ void testGetFile() {
assertThrows(UnsupportedOperationException.class, super::testGetFile);
}
- @Override
Review Comment:
This method doesn't need to be moved; it makes the PR larger for no reason.
##########
src/test/java/org/apache/commons/io/build/ReaderOriginTest.java:
##########
@@ -56,13 +57,6 @@ void testGetFile() {
assertThrows(UnsupportedOperationException.class, super::testGetFile);
}
- @Override
Review Comment:
This method doesn't need to be moved; it makes the PR larger for no reason.
##########
src/test/java/org/apache/commons/io/build/WriterStreamOriginTest.java:
##########
@@ -89,13 +90,6 @@ void testGetFile() {
assertThrows(UnsupportedOperationException.class, super::testGetFile);
}
- @Override
Review Comment:
This method doesn't need to be moved; it makes the PR larger for no reason.
##########
src/test/java/org/apache/commons/io/channels/ByteArraySeekableByteChannelCompressTest.java:
##########
@@ -62,7 +66,7 @@ void testCloseIsIdempotent() throws Exception {
@ParameterizedTest
@ValueSource(ints = { 0, 1, 2, 3, 4, 5, 6 })
void testReadingFromAPositionAfterEndReturnsEOF(final int size) throws Exception {
- try (SeekableByteChannel c = new ByteArraySeekableByteChannel(size)) {
+ try (SeekableByteChannel c = ByteArraySeekableByteChannel.wrap(new byte[size])) {
Review Comment:
Sure, but I am talking about the _all_ the tests that read from a byte array like the ones below. I should have made that clearer in my initial comment. For example, this is _not_ the pattern we want:
```diff
@Test
void testShouldReadContentsProperly() throws IOException {
- try (ByteArraySeekableByteChannel c = new ByteArraySeekableByteChannel(testData)) {
+ try (ByteArraySeekableByteChannel c = ByteArraySeekableByteChannel.wrap(cloneTestData())) {
```
A read test should not affect the underlying byte array so you want to give it the raw array.
If we want to give it a copy, then maybe the PR should also make sure the copy has not been altered?
##########
src/test/java/org/apache/commons/io/build/CharSequenceOriginTest.java:
##########
@@ -109,11 +123,4 @@ void testGetReaderIgnoreCharsetNull() throws IOException {
}
}
- @Override
Review Comment:
It's confusing to see a method deleted and added again.
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,74 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
*/
public class ByteArraySeekableByteChannel implements SeekableByteChannel {
private static final int RESIZE_LIMIT = Integer.MAX_VALUE >> 1;
+
+ /**
+ * Constructs a new channel backed directly by the given byte array.
+ *
+ * <p>The channel initially contains the full contents of the array, with its
+ * size set to {@code bytes.length} and its position set to {@code 0}.</p>
+ *
+ * <p>Reads and writes operate on the shared array.
+ * If a write operation extends beyond the current capacity, the channel will
+ * automatically allocate a larger backing array and copy the existing contents.</p>
+ *
+ * @param bytes The byte array to wrap, must not be {@code null}
+ * @return A new channel that uses the given array as its initial backing store
+ * @throws NullPointerException If {@code bytes} is {@code null}
+ * @see #array()
+ * @see java.io.ByteArrayInputStream#ByteArrayInputStream(byte[])
+ */
+ public static ByteArraySeekableByteChannel wrap(byte[] bytes) {
+ Objects.requireNonNull(bytes, "bytes");
+ return new ByteArraySeekableByteChannel(bytes);
+ }
+
private byte[] data;
- private final AtomicBoolean closed = new AtomicBoolean();
+ private volatile boolean closed;
Review Comment:
If you "Resolve a Conversation" in GH after answering a question, the person asking the question doesn't see the answer! It's hidden behind a "Show resolved" button. I think it makes sense for the person asking the question to do the "Resolve". Now I have to open every "Show resolved" to see if a question was truly resolved or not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2366416013
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -203,7 +452,7 @@ public CharSequenceOrigin(final CharSequence origin) {
@Override
public byte[] getByteArray() {
// TODO Pass in a Charset? Consider if call sites actually need this.
- return origin.toString().getBytes(Charset.defaultCharset());
+ return getCharSequence(null).toString().getBytes(Charset.defaultCharset());
Review Comment:
How is this change related to Channels? This is a giant PR already. Can it be focused on just the new feature? Or is this a bug fix?
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -267,7 +521,7 @@ public FileOrigin(final File origin) {
@Override
public byte[] getByteArray(final long position, final int length) throws IOException {
- try (RandomAccessFile raf = RandomAccessFileMode.READ_ONLY.create(origin)) {
+ try (RandomAccessFile raf = RandomAccessFileMode.READ_ONLY.create(getFile())) {
Review Comment:
How is this change related to Channels? This is a giant PR already. Can it be focused on just the new feature? Or is this a bug fix?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2371390895
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,74 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
Review Comment:
I have Copilot enabled in my IDE, so I am always watching for this kind of typos: Copilot always suggests a random completion of `@since` tags.
BTW: Copilot also suggests random completions for SHA256 properties. :laughing:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2371462120
##########
src/test/java/org/apache/commons/io/channels/ByteArraySeekableByteChannelCompressTest.java:
##########
@@ -42,6 +42,10 @@ class ByteArraySeekableByteChannelCompressTest {
private final byte[] testData = "Some data".getBytes(StandardCharsets.UTF_8);
+ private byte[] getTestData() {
Review Comment:
Fixed in https://github.com/apache/commons-io/pull/784/commits/42133ae14f0d227064f4c320b87ab8a68b588185
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2379044733
##########
src/test/java/org/apache/commons/io/build/CharSequenceOriginTest.java:
##########
@@ -62,13 +62,6 @@ void testGetFile() {
assertThrows(UnsupportedOperationException.class, super::testGetFile);
}
- @Override
- @Test
- void testGetOutputStream() {
Review Comment:
This helped me check whether each `✖` in the `AbstractOrigin` support matrix has a corresponding test, but you are right, I shouldn't have changed the order of the existing methods.
Note that keeping alphabetical order is also not ideal as it separates related methods: I usually put the `static` method that generates parameters for a parameterized test near the test method. Reordering methods with Eclipse breaks them apart.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2368026868
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -324,6 +589,18 @@ public Reader getReader(final Charset charset) throws IOException {
return new InputStreamReader(getInputStream(), Charsets.toCharset(charset));
}
+ @Override
+ public ReadableByteChannel getReadableByteChannel(OpenOption... options) throws IOException {
+ return Channels.newChannel(getInputStream(options));
+ }
+
+ public long size() throws IOException {
+ if (origin instanceof FileInputStream) {
+ final FileInputStream fileInputStream = (FileInputStream) origin;
+ return fileInputStream.getChannel().size();
+ }
+ throw unsupportedOperation("size");
+ }
Review Comment:
Fixed in https://github.com/apache/commons-io/pull/784/commits/a2dd032ef042b0fa35bf68ba49cd66636bf2d3bd
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2368184174
##########
src/main/java/org/apache/commons/io/build/AbstractOriginSupplier.java:
##########
@@ -368,4 +376,26 @@ public B setURI(final URI origin) {
public B setWriter(final Writer origin) {
return setOrigin(newWriterOrigin(origin));
}
+
+ /**
+ * Sets a new origin.
+ *
+ * @param origin the new origin.
+ * @return {@code this} instance.
+ * @since 2.21.0
+ */
+ public B setReadableByteChannel(final ReadableByteChannel origin) {
+ return setOrigin(newChannelOrigin(origin));
+ }
+
+ /**
+ * Sets a new origin.
+ *
+ * @param origin the new origin.
+ * @return {@code this} instance.
+ * @since 2.21.0
+ */
+ public B setWritableByteChannel(final WritableByteChannel origin) {
Review Comment:
Well, thinking of the Compress use case, the only interface that matters there is `SeekableByteChannel` which is both a read and write channel as you pointed out.
>On the other hand, many channel implementations declare both interfaces but only fully support one.
I don't think we should be concerned about implementations since we are supporting interfaces here. This feels a bit like `Path` which is an abstract class but behaves like an interface for call sites. We don't concern ourselves with the implementations of `Path`.
I would expect specific origins to test for read/write/seekable kinds of channels and use what they need.
So all of this to say it seems like you should be able to say `setChannel(Channel)` and getters like `get ReadableByteChannel()`, `getSeekableByteChannel()`, and `getSeekableByteChannel()` should do their best or throw.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2367837278
##########
src/changes/changes.xml:
##########
@@ -62,10 +62,12 @@ The <action> type attribute can be add,update,fix,remove.
<action dev="ggregory" type="add" due-to="Gary Gregory">Add org.apache.commons.io.output.ProxyOutputStream.writeRepeat(int, long).</action>
<action dev="pkarwasz" type="add" due-to="Piotr P. Karwasz">Add length unit support in FileSystem limits.</action>
<action dev="pkarwasz" type="add" due-to="Piotr P. Karwasz">Add IOUtils.toByteArray(InputStream, int, int) for safer chunked reading with size validation.</action>
- <action dev="pkarwasz" type="add" due-to="Piotr P. Karwasz">Add org.apache.commons.io.file.PathUtils.getPath(String, String).</action>
+ <action dev="ggregory" type="add" due-to="Gary Gregory">Add org.apache.commons.io.file.PathUtils.getPath(String, String).</action>
<action dev="ggregory" type="add" due-to="Gary Gregory">Add org.apache.commons.io.channels.ByteArraySeekableByteChannel.</action>
+ <action dev="pkarwasz" type="add" due-to="Piotr P. Karwasz">Add ByteArrayChannel, a simple in-memory `SeekableByteChannel` implementation.</action>
+ <action dev="pkarwasz" type="add" due-to="Piotr P. Karwasz">Add NIO channel support to `AbstractStreamBuilder`.</action>
<!-- UPDATE -->
- <action type="update" dev="ggregory" due-to="Gary Gregory, Dependabot">Bump org.apache.commons:commons-parent from 85 to 88 #774, #783.</action>
+ <action type="update" dev="ggregory" due-to="Gary Gregory, Dependabot">Bump org.apache.commons:commons-parent from 85 to 87 #774.</action>
Review Comment:
This was some merge issue. I fixed it in https://github.com/apache/commons-io/pull/784/commits/32cb23976910a368985ce02025c14378d344651f
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2366415963
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -227,7 +476,7 @@ public CharSequence getCharSequence(final Charset charset) {
@Override
public InputStream getInputStream(final OpenOption... options) throws IOException {
// TODO Pass in a Charset? Consider if call sites actually need this.
- return CharSequenceInputStream.builder().setCharSequence(getCharSequence(Charset.defaultCharset())).get();
+ return CharSequenceInputStream.builder().setCharSequence(getCharSequence(null)).get();
Review Comment:
How is this change related to Channels? This is a giant PR already. Can it be focused on just the new feature? Or is this a bug fix?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2367073492
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -137,6 +372,16 @@ public Writer getWriter(final Charset charset, final OpenOption... options) thro
return new OutputStreamWriter(getOutputStream(options), Charsets.toCharset(charset));
}
+ @Override
+ public ReadableByteChannel getReadableByteChannel(OpenOption... options) throws IOException {
+ return getRandomAccessFile(options).getChannel();
Review Comment:
Good point! :100:
Should we introduce an `AbstractFileBasedOrigin` that introduces a `getFileChannel()` method? Some origins support that (`RandomAccessFile`, `File`, `Path` and possibly `FileInputStream/FileOutputStream`) but others don't.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #784:
URL: https://github.com/apache/commons-io/pull/784#issuecomment-3320646816
> > Resource ownership semantics are unclear. Some origins wrap pre-opened resources (InputStream, RandomAccessFile, Channel), others create new resources on demand (byte[], File, Path, URI). Right now it’s ambiguous who “owns” the underlying resource. In practice, the builder should not take ownership unless explicitly documented; otherwise returned resources should be close-shielded.
>
> I thought this was all clear to: The allocator owns the resources and must close it.
>
> Are there any bugs?
If the intent is that the allocator always owns and closes the resource, then we do have a problem: the current implementations don’t consistently enforce that contract. For example, resources returned from `AbstractOrigin` are not close-shielded, so callers can unintentionally close the underlying origin.
A concrete case:
```java
InputStream original = ...
InputStreamOrigin origin = new InputStreamOrigin(original);
try (InputStream input = origin.getInputStream()) {
// Do something
}
// original is closed unexpectedly
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2379047303
##########
src/test/java/org/apache/commons/io/build/WriterStreamOriginTest.java:
##########
@@ -89,13 +90,6 @@ void testGetFile() {
assertThrows(UnsupportedOperationException.class, super::testGetFile);
}
- @Override
Review Comment:
Fixed in https://github.com/apache/commons-io/pull/784/commits/46235ff46e47dcb5fbf79b9c51ce64f193956e39
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #784:
URL: https://github.com/apache/commons-io/pull/784#issuecomment-3335881732
@garydgregory, done!
FYI: the `feat:` in my titles is a reference to [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/). I use these prefixes often in my commits, the message of the first commit is usually used to fill the PR and after squashing, it becomes the title of a commit on `master`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2379046655
##########
src/test/java/org/apache/commons/io/build/OutputStreamOriginTest.java:
##########
@@ -89,13 +90,6 @@ void testGetFile() {
assertThrows(UnsupportedOperationException.class, super::testGetFile);
}
- @Override
Review Comment:
Fixed in https://github.com/apache/commons-io/pull/784/commits/46235ff46e47dcb5fbf79b9c51ce64f193956e39
##########
src/test/java/org/apache/commons/io/build/ReaderOriginTest.java:
##########
@@ -56,13 +57,6 @@ void testGetFile() {
assertThrows(UnsupportedOperationException.class, super::testGetFile);
}
- @Override
Review Comment:
Fixed in https://github.com/apache/commons-io/pull/784/commits/46235ff46e47dcb5fbf79b9c51ce64f193956e39
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2379045722
##########
src/test/java/org/apache/commons/io/build/CharSequenceOriginTest.java:
##########
@@ -109,11 +123,4 @@ void testGetReaderIgnoreCharsetNull() throws IOException {
}
}
- @Override
Review Comment:
Fixed in https://github.com/apache/commons-io/pull/784/commits/46235ff46e47dcb5fbf79b9c51ce64f193956e39
##########
src/test/java/org/apache/commons/io/build/InputStreamOriginTest.java:
##########
@@ -58,13 +59,6 @@ void testGetFile() {
assertThrows(UnsupportedOperationException.class, super::testGetFile);
}
- @Override
Review Comment:
Fixed in https://github.com/apache/commons-io/pull/784/commits/46235ff46e47dcb5fbf79b9c51ce64f193956e39
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2366411889
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,67 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
*/
public class ByteArraySeekableByteChannel implements SeekableByteChannel {
private static final int RESIZE_LIMIT = Integer.MAX_VALUE >> 1;
- private byte[] data;
- private final AtomicBoolean closed = new AtomicBoolean();
+
+ /**
+ * Constructs a channel that wraps the given byte array.
+ * <p>
+ * The resulting channel will share the given array as its buffer, until a write operation
+ * requires a larger capacity.
+ * The initial size of the channel is the length of the given array, and the initial position is 0.
+ * </p>
+ * @param bytes The byte array to wrap; must not be {@code null}.
+ * @return A new channel that wraps the given byte array; never {@code null}.
+ * @throws NullPointerException If the byte array is {@code null}.
+ */
+ public static ByteArraySeekableByteChannel wrap(byte[] bytes) {
+ Objects.requireNonNull(bytes, "bytes");
+ return new ByteArraySeekableByteChannel(bytes, bytes.length);
+ }
+
+ // package-private for testing
+ byte[] data;
Review Comment:
Please leave as private and use the `array()` method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2371461275
##########
src/test/java/org/apache/commons/io/channels/ByteArraySeekableByteChannelCompressTest.java:
##########
@@ -62,7 +66,7 @@ void testCloseIsIdempotent() throws Exception {
@ParameterizedTest
@ValueSource(ints = { 0, 1, 2, 3, 4, 5, 6 })
void testReadingFromAPositionAfterEndReturnsEOF(final int size) throws Exception {
- try (SeekableByteChannel c = new ByteArraySeekableByteChannel(size)) {
+ try (SeekableByteChannel c = ByteArraySeekableByteChannel.wrap(new byte[size])) {
Review Comment:
This change is due to the different semantics of `wrap` vs the constructor: `wrap` also updates the logical size of the channel to the length of the byte array, while the constructor keeps it at 0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2366836910
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,67 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
*/
public class ByteArraySeekableByteChannel implements SeekableByteChannel {
private static final int RESIZE_LIMIT = Integer.MAX_VALUE >> 1;
- private byte[] data;
- private final AtomicBoolean closed = new AtomicBoolean();
+
+ /**
+ * Constructs a channel that wraps the given byte array.
+ * <p>
+ * The resulting channel will share the given array as its buffer, until a write operation
+ * requires a larger capacity.
+ * The initial size of the channel is the length of the given array, and the initial position is 0.
+ * </p>
+ * @param bytes The byte array to wrap; must not be {@code null}.
+ * @return A new channel that wraps the given byte array; never {@code null}.
+ * @throws NullPointerException If the byte array is {@code null}.
+ */
+ public static ByteArraySeekableByteChannel wrap(byte[] bytes) {
+ Objects.requireNonNull(bytes, "bytes");
+ return new ByteArraySeekableByteChannel(bytes, bytes.length);
+ }
+
+ // package-private for testing
+ byte[] data;
Review Comment:
Fixed in https://github.com/apache/commons-io/pull/784/commits/0c853487783a43abb85de3fcc204ecca3a5ca421
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2367858008
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -37,41 +37,66 @@
* accessed via {@link ByteArraySeekableByteChannel#array()}.
* </p>
*
- * @since 2.19.0
+ * @since 2.21.0
*/
public class ByteArraySeekableByteChannel implements SeekableByteChannel {
private static final int RESIZE_LIMIT = Integer.MAX_VALUE >> 1;
+
+ /**
+ * Constructs a channel that wraps the given byte array.
+ * <p>
+ * The resulting channel will share the given array as its buffer, until a write operation
+ * requires a larger capacity.
+ * The initial size of the channel is the length of the given array, and the initial position is 0.
+ * </p>
+ * @param bytes The byte array to wrap; must not be {@code null}.
+ * @return A new channel that wraps the given byte array; never {@code null}.
+ * @throws NullPointerException If the byte array is {@code null}.
+ */
+ public static ByteArraySeekableByteChannel wrap(byte[] bytes) {
+ Objects.requireNonNull(bytes, "bytes");
+ return new ByteArraySeekableByteChannel(bytes, bytes.length);
+ }
+
private byte[] data;
- private final AtomicBoolean closed = new AtomicBoolean();
+ private volatile boolean closed;
private int position;
private int size;
private final ReentrantLock lock = new ReentrantLock();
/**
* Constructs a new instance using a default empty buffer.
+ * <p>
+ * The initial size of the channel is 0, and the initial position is 0.
+ * </p>
*/
public ByteArraySeekableByteChannel() {
this(0);
}
- /**
- * Constructs a new instance from a byte array.
- *
- * @param data input data or pre-allocated array.
- */
- public ByteArraySeekableByteChannel(final byte[] data) {
- this.data = data;
- this.size = data.length;
- }
-
/**
* Constructs a new instance from a size of storage to be allocated.
- *
+ * <p>
+ * The initial size of the channel is 0, and the initial position is 0.
+ * </p>
* @param size size of internal buffer to allocate, in bytes.
*/
public ByteArraySeekableByteChannel(final int size) {
- this(new byte[size]);
+ this(byteArray(size), 0);
+ }
+
+ private static byte[] byteArray(int value) {
+ if (value < 0) {
+ throw new IllegalArgumentException("Size must be non-negative");
+ }
+ return new byte[value];
+ }
+
+ private ByteArraySeekableByteChannel(byte[] data, int size) {
Review Comment:
The private constructor doesn't validate that the size parameter is within bounds of the data array length. Consider adding validation to ensure size <= data.length to prevent inconsistent internal state.
```suggestion
private ByteArraySeekableByteChannel(byte[] data, int size) {
if (size < 0 || size > data.length) {
throw new IllegalArgumentException("Size must be non-negative and less than or equal to data.length (size=" + size + ", data.length=" + data.length + ")");
}
```
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -212,13 +249,12 @@ public int write(final ByteBuffer b) throws IOException {
checkOpen();
lock.lock();
try {
- int wanted = b.remaining();
- final int possibleWithoutResize = size - position;
+ final int wanted = b.remaining();
+ final int possibleWithoutResize = Math.max(0, size - position);
Review Comment:
The calculation `Math.max(0, size - position)` is unnecessary since position should never exceed size in a properly functioning channel. Consider using a simpler calculation or adding an assertion to verify this invariant.
```suggestion
assert position <= size : "Channel position exceeds size";
final int possibleWithoutResize = size - position;
```
##########
src/main/java/org/apache/commons/io/build/AbstractOrigin.java:
##########
@@ -324,6 +589,18 @@ public Reader getReader(final Charset charset) throws IOException {
return new InputStreamReader(getInputStream(), Charsets.toCharset(charset));
}
+ @Override
+ public ReadableByteChannel getReadableByteChannel(OpenOption... options) throws IOException {
+ return Channels.newChannel(getInputStream(options));
+ }
+
+ public long size() throws IOException {
+ if (origin instanceof FileInputStream) {
+ final FileInputStream fileInputStream = (FileInputStream) origin;
+ return fileInputStream.getChannel().size();
+ }
+ throw unsupportedOperation("size");
+ }
Review Comment:
The size() method in InputStreamOrigin is missing the @Override annotation. This should be added for clarity and to ensure proper method overriding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2368021120
##########
src/main/java/org/apache/commons/io/channels/ByteArraySeekableByteChannel.java:
##########
@@ -212,13 +249,12 @@ public int write(final ByteBuffer b) throws IOException {
checkOpen();
lock.lock();
try {
- int wanted = b.remaining();
- final int possibleWithoutResize = size - position;
+ final int wanted = b.remaining();
+ final int possibleWithoutResize = Math.max(0, size - position);
Review Comment:
The `SeekableByteChannel` contract explicitly allows `position` to be larger than `size`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: add NIO channel support to `AbstractStreamBuilder` [commons-io]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #784:
URL: https://github.com/apache/commons-io/pull/784#discussion_r2368133774
##########
src/test/java/org/apache/commons/io/build/ByteArrayOriginTest.java:
##########
@@ -82,11 +75,24 @@ void testGetRandomAccessFile(final OpenOption openOption) {
assertThrows(UnsupportedOperationException.class, () -> super.testGetRandomAccessFile(openOption));
}
+ @Override
+ @Test
+ void testGetOutputStream() {
+ // Cannot convert a byte[] to an OutputStream.
+ assertThrows(UnsupportedOperationException.class, super::testGetOutputStream);
Review Comment:
Good analysis, ty. Since this use case has not come up, I think we should be good for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org