You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by pk...@apache.org on 2025/10/13 13:58:30 UTC
(commons-io) branch fix/close-shield-channel-interface-detection created (now 3f4dae73e)
This is an automated email from the ASF dual-hosted git repository.
pkarwasz pushed a change to branch fix/close-shield-channel-interface-detection
in repository https://gitbox.apache.org/repos/asf/commons-io.git
at 3f4dae73e Fixes issues in `CloseShieldChannel`
This branch includes the following new commits:
new 3f4dae73e Fixes issues in `CloseShieldChannel`
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(commons-io) 01/01: Fixes issues in `CloseShieldChannel`
Posted by pk...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
pkarwasz pushed a commit to branch fix/close-shield-channel-interface-detection
in repository https://gitbox.apache.org/repos/asf/commons-io.git
commit 3f4dae73e67d329da3b70811d2103394ec0dde9a
Author: Piotr P. Karwasz <pk...@apache.org>
AuthorDate: Mon Oct 13 15:57:57 2025 +0200
Fixes issues in `CloseShieldChannel`
Two bugs in the `CloseShieldChannel` helper make it unreliable in practice:
1. **Type-erasure bug in `T wrap(T)`**
The method signature only works correctly when `T` is an **interface** extending `Channel`.
Since Java’s type system doesn’t allow constraining `T` to “interface types only,” this could lead to unexpected runtime `ClassCastException`s even though the code compiles successfully.
2. **Incomplete interface discovery**
The implementation only inspected interfaces **directly** implemented by the given channel’s class, ignoring those inherited from its superclasses.
As a result, proxies for types such as `FileChannel` did not expose any of the interfaces declared on `FileChannel` itself.
#### Fixes
This PR addresses both issues:
* **Reworks the API signature**
* Replaces `T wrap(T)` with its erasure: `Channel wrap(Channel)`.
* Introduces a new overload: `T wrap(T, Class<T>)`, which allows callers to explicitly specify the interface type they expect.
This version fails fast with a clear `IllegalArgumentException` if the provided type is not an interface, instead of allowing a `ClassCastException` later.
* **Improves interface collection logic**
* Updates the implementation to include interfaces declared on superclasses, ensuring all relevant `Channel` interfaces are correctly proxied.
---
.../commons/io/channels/CloseShieldChannel.java | 37 ++++++++++---
.../io/channels/CloseShieldChannelTest.java | 63 +++++++++++++++++++---
2 files changed, 86 insertions(+), 14 deletions(-)
diff --git a/src/main/java/org/apache/commons/io/channels/CloseShieldChannel.java b/src/main/java/org/apache/commons/io/channels/CloseShieldChannel.java
index a9a462da7..7be297e20 100644
--- a/src/main/java/org/apache/commons/io/channels/CloseShieldChannel.java
+++ b/src/main/java/org/apache/commons/io/channels/CloseShieldChannel.java
@@ -40,11 +40,15 @@ public final class CloseShieldChannel {
private static final Class<?>[] EMPTY = {};
private static Set<Class<?>> collectChannelInterfaces(final Class<?> type, final Set<Class<?>> out) {
+ Class<?> currentType = type;
// Visit interfaces
- for (final Class<?> iface : type.getInterfaces()) {
- if (Channel.class.isAssignableFrom(iface) && out.add(iface)) {
- collectChannelInterfaces(iface, out);
+ while (currentType != null) {
+ for (final Class<?> iface : currentType.getInterfaces()) {
+ if (Channel.class.isAssignableFrom(iface) && out.add(iface)) {
+ collectChannelInterfaces(iface, out);
+ }
}
+ currentType = currentType.getSuperclass();
}
return out;
}
@@ -53,11 +57,10 @@ private static Set<Class<?>> collectChannelInterfaces(final Class<?> type, final
* Wraps a channel to shield it from being closed.
*
* @param channel The underlying channel to shield, not {@code null}.
- * @param <T> Any Channel type (interface or class).
* @return A proxy that shields {@code close()} and enforces closed semantics on other calls.
*/
@SuppressWarnings({ "unchecked", "resource" }) // caller closes
- public static <T extends Channel> T wrap(final T channel) {
+ public static Channel wrap(final Channel channel) {
Objects.requireNonNull(channel, "channel");
// Fast path: already our shield
if (Proxy.isProxyClass(channel.getClass()) && Proxy.getInvocationHandler(channel) instanceof CloseShieldChannelHandler) {
@@ -66,10 +69,32 @@ public static <T extends Channel> T wrap(final T channel) {
// Collect only Channel sub-interfaces.
final Set<Class<?>> set = collectChannelInterfaces(channel.getClass(), new LinkedHashSet<>());
// fallback to root surface
- return (T) Proxy.newProxyInstance(channel.getClass().getClassLoader(), // use delegate's loader
+ return (Channel) Proxy.newProxyInstance(channel.getClass().getClassLoader(), // use delegate's loader
set.isEmpty() ? new Class<?>[] { Channel.class } : set.toArray(EMPTY), new CloseShieldChannelHandler(channel));
}
+ /**
+ * Wraps a channel to shield it from being closed.
+ *
+ * @param channel The underlying channel to shield, not {@code null} and must implement {@code type}.
+ * @param type The interface the returned proxy must implement;
+ * the proxy will also implement all other {@link Channel} sub-interfaces that the delegate implements.
+ * @param <T> A type that extends {@link Channel}.
+ * @return A proxy that shields {@code close()} and enforces closed semantics on other calls.
+ * @throws IllegalArgumentException if {@code type} is not an interface or if {@code channel} does not implement {@code type}.
+ */
+ @SuppressWarnings({ "unchecked", "resource" }) // caller closes
+ public static <T extends Channel> T wrap(final T channel, Class<T> type) {
+ Objects.requireNonNull(type, "type");
+ if (!type.isInterface()) {
+ throw new IllegalArgumentException(type.getName() + " is not an interface");
+ }
+ if (!type.isInstance(channel)) {
+ throw new IllegalArgumentException("channel of type " + channel.getClass().getName() + " is not an instance of " + type.getName());
+ }
+ return type.cast(wrap(channel));
+ }
+
private CloseShieldChannel() {
// no instance
}
diff --git a/src/test/java/org/apache/commons/io/channels/CloseShieldChannelTest.java b/src/test/java/org/apache/commons/io/channels/CloseShieldChannelTest.java
index ac7e85b7a..6e15ba02b 100644
--- a/src/test/java/org/apache/commons/io/channels/CloseShieldChannelTest.java
+++ b/src/test/java/org/apache/commons/io/channels/CloseShieldChannelTest.java
@@ -20,6 +20,7 @@
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
import static org.junit.jupiter.api.Assertions.assertNotSame;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -32,11 +33,13 @@
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;
+import java.io.IOException;
import java.nio.channels.AsynchronousByteChannel;
import java.nio.channels.AsynchronousChannel;
import java.nio.channels.ByteChannel;
import java.nio.channels.Channel;
import java.nio.channels.ClosedChannelException;
+import java.nio.channels.FileChannel;
import java.nio.channels.GatheringByteChannel;
import java.nio.channels.InterruptibleChannel;
import java.nio.channels.MulticastChannel;
@@ -45,9 +48,12 @@
import java.nio.channels.ScatteringByteChannel;
import java.nio.channels.SeekableByteChannel;
import java.nio.channels.WritableByteChannel;
+import java.nio.file.Path;
import java.util.stream.Stream;
+import org.apache.commons.io.FileUtils;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.MethodSource;
@@ -116,8 +122,8 @@ void testCloseIsShielded(final Class<? extends Channel> channelClass) throws Exc
@Test
void testDoesNotDoubleWrap() {
final ByteChannel channel = mock(ByteChannel.class);
- final ByteChannel shield1 = CloseShieldChannel.wrap(channel);
- final ByteChannel shield2 = CloseShieldChannel.wrap(shield1);
+ final ByteChannel shield1 = CloseShieldChannel.wrap(channel, ByteChannel.class);
+ final ByteChannel shield2 = CloseShieldChannel.wrap(shield1, ByteChannel.class);
assertSame(shield1, shield2);
}
@@ -137,7 +143,7 @@ void testEquals(final Class<? extends Channel> channelClass) throws Exception {
void testGatheringByteChannelMethods() throws Exception {
final GatheringByteChannel channel = mock(GatheringByteChannel.class);
when(channel.isOpen()).thenReturn(true);
- final GatheringByteChannel shield = CloseShieldChannel.wrap(channel);
+ final GatheringByteChannel shield = CloseShieldChannel.wrap(channel, GatheringByteChannel.class);
// Before close write() should delegate
when(channel.write(null, 0, 0)).thenReturn(42L);
assertEquals(42, shield.write(null, 0, 0));
@@ -162,7 +168,7 @@ void testHashCode(final Class<? extends Channel> channelClass) throws Exception
void testNetworkChannelMethods() throws Exception {
final NetworkChannel channel = mock(NetworkChannel.class);
when(channel.isOpen()).thenReturn(true);
- final NetworkChannel shield = CloseShieldChannel.wrap(channel);
+ final NetworkChannel shield = CloseShieldChannel.wrap(channel, NetworkChannel.class);
// Before close getOption(), setOption(), getLocalAddress() and bind() should delegate
when(channel.getOption(null)).thenReturn("foo");
when(channel.setOption(null, null)).thenReturn(channel);
@@ -201,7 +207,7 @@ void testPreservesInterfaces(final Class<? extends Channel> channelClass) {
void testReadableByteChannelMethods() throws Exception {
final ReadableByteChannel channel = mock(ReadableByteChannel.class);
when(channel.isOpen()).thenReturn(true);
- final ReadableByteChannel shield = CloseShieldChannel.wrap(channel);
+ final ReadableByteChannel shield = CloseShieldChannel.wrap(channel, ReadableByteChannel.class);
// Before close read() should delegate
when(channel.read(null)).thenReturn(42);
assertEquals(42, shield.read(null));
@@ -216,7 +222,7 @@ void testReadableByteChannelMethods() throws Exception {
void testScatteringByteChannelMethods() throws Exception {
final ScatteringByteChannel channel = mock(ScatteringByteChannel.class);
when(channel.isOpen()).thenReturn(true);
- final ScatteringByteChannel shield = CloseShieldChannel.wrap(channel);
+ final ScatteringByteChannel shield = CloseShieldChannel.wrap(channel, ScatteringByteChannel.class);
// Before close read() should delegate
when(channel.read(null, 0, 0)).thenReturn(42L);
assertEquals(42, shield.read(null, 0, 0));
@@ -231,7 +237,7 @@ void testScatteringByteChannelMethods() throws Exception {
void testSeekableByteChannelMethods() throws Exception {
final SeekableByteChannel channel = mock(SeekableByteChannel.class);
when(channel.isOpen()).thenReturn(true);
- final SeekableByteChannel shield = CloseShieldChannel.wrap(channel);
+ final SeekableByteChannel shield = CloseShieldChannel.wrap(channel, SeekableByteChannel.class);
// Before close position() and size() should delegate
when(channel.position()).thenReturn(42L);
when(channel.size()).thenReturn(84L);
@@ -270,7 +276,7 @@ void testToString(final Class<? extends Channel> channelClass) throws Exception
void testWritableByteChannelMethods() throws Exception {
final WritableByteChannel channel = mock(WritableByteChannel.class);
when(channel.isOpen()).thenReturn(true);
- final WritableByteChannel shield = CloseShieldChannel.wrap(channel);
+ final WritableByteChannel shield = CloseShieldChannel.wrap(channel, WritableByteChannel.class);
// Before close write() should delegate
when(channel.write(null)).thenReturn(42);
assertEquals(42, shield.write(null));
@@ -280,4 +286,45 @@ void testWritableByteChannelMethods() throws Exception {
assertThrows(ClosedChannelException.class, () -> shield.write(null));
verifyNoMoreInteractions(channel);
}
+
+ @Test
+ void testCorrectlyDetectsInterfaces(@TempDir Path tempDir) throws IOException {
+ final Path testFile = tempDir.resolve("test.txt");
+ FileUtils.touch(testFile.toFile());
+ try (FileChannel channel = FileChannel.open(testFile); Channel shield = CloseShieldChannel.wrap(channel)) {
+ assertInstanceOf(SeekableByteChannel.class, shield);
+ assertInstanceOf(GatheringByteChannel.class, shield);
+ assertInstanceOf(WritableByteChannel.class, shield);
+ assertInstanceOf(ScatteringByteChannel.class, shield);
+ assertInstanceOf(ReadableByteChannel.class, shield);
+ assertInstanceOf(InterruptibleChannel.class, shield);
+ assertInstanceOf(ByteChannel.class, shield);
+ assertInstanceOf(Channel.class, shield);
+ // These are not interfaces, so can not be implemented
+ assertFalse(shield instanceof FileChannel, "not FileChannel");
+ }
+ }
+
+ @Test
+ void testThrowsIllegalArgumentException(@TempDir Path tempDir) throws Exception {
+ final Path testFile = tempDir.resolve("test.txt");
+ FileUtils.touch(testFile.toFile());
+ // Argument is not an interface
+ try (FileChannel channel = FileChannel.open(testFile)) {
+ final IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, () -> CloseShieldChannel.wrap(channel, FileChannel.class));
+ assertTrue(ex.getMessage().contains("not an interface"));
+ }
+ // Argument is not an instance of the specified interface.
+ //
+ // This situation is rare in normal code because the compiler enforces type compatibility
+ // when T is known at compile time. However, it can still occur at runtime if a value is
+ // passed through an unchecked cast or comes from a raw type, bypassing generic type checks.
+ try (Channel channel = mock(Channel.class)) {
+ @SuppressWarnings("rawtypes")
+ final Class channelClass = ReadableByteChannel.class;
+ @SuppressWarnings("unchecked")
+ final IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, () -> CloseShieldChannel.wrap(channel, channelClass));
+ assertTrue(ex.getMessage().contains("not an instance of"));
+ }
+ }
}