Bill commented on a change in pull request #5363:
URL: https://github.com/apache/geode/pull/5363#discussion_r454733308
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/net/SocketUtilsJUnitTest.java
##########
@@ -88,4 +95,86 @@ public void testCloseServerSocketThrowsIOException() throws
IOException {
public void testCloseServerSocketWithNull() {
assertThat(SocketUtils.close((ServerSocket) null)).isTrue();
}
+
+ @Test
+ public void readFromSocketWithHeapBuffer() throws IOException {
+ Socket socket = mock(Socket.class);
+ SocketChannel channel = mock(SocketChannel.class);
+ when(socket.getChannel()).thenReturn(channel);
+ final ByteBuffer buffer = ByteBuffer.allocate(100); // heap buffer
+ byte[] bytes = new byte[100];
+ InputStream stream = new ByteArrayInputStream(bytes);
+ when(channel.read(buffer)).thenAnswer((answer) -> {
+ buffer.put(bytes);
+ return buffer.position();
+ });
+ assertThat(buffer.hasArray()).isTrue();
+ SocketUtils.readFromSocket(socket, buffer, stream);
+ // the channel was used to read the bytes
+ verify(channel, times(1)).read(buffer);
+ // the buffer was filled
+ assertThat(buffer.position()).isEqualTo(bytes.length);
+ // the stream was not used
+ assertThat(stream.available()).isEqualTo(bytes.length);
+ }
+
+
+ @Test
+ public void readFromSocketWithDirectBuffer() throws IOException {
+ Socket socket = mock(Socket.class);
+ SocketChannel channel = mock(SocketChannel.class);
+ when(socket.getChannel()).thenReturn(channel);
+ final ByteBuffer buffer = ByteBuffer.allocateDirect(100); // non-heap
buffer
Review comment:
Great to see this increased code coverage!
From scanning this method and the previous one I think the only two
differences are 1) the allocation statement and 2) the verification of
`buffer.hasArray()`. It would aid maintenance, I think, if the parts that are
identical were factored out.
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/net/SocketUtilsJUnitTest.java
##########
@@ -88,4 +95,86 @@ public void testCloseServerSocketThrowsIOException() throws
IOException {
public void testCloseServerSocketWithNull() {
assertThat(SocketUtils.close((ServerSocket) null)).isTrue();
}
+
+ @Test
+ public void readFromSocketWithHeapBuffer() throws IOException {
+ Socket socket = mock(Socket.class);
+ SocketChannel channel = mock(SocketChannel.class);
+ when(socket.getChannel()).thenReturn(channel);
+ final ByteBuffer buffer = ByteBuffer.allocate(100); // heap buffer
+ byte[] bytes = new byte[100];
+ InputStream stream = new ByteArrayInputStream(bytes);
+ when(channel.read(buffer)).thenAnswer((answer) -> {
+ buffer.put(bytes);
+ return buffer.position();
+ });
+ assertThat(buffer.hasArray()).isTrue();
+ SocketUtils.readFromSocket(socket, buffer, stream);
+ // the channel was used to read the bytes
+ verify(channel, times(1)).read(buffer);
+ // the buffer was filled
+ assertThat(buffer.position()).isEqualTo(bytes.length);
+ // the stream was not used
+ assertThat(stream.available()).isEqualTo(bytes.length);
+ }
+
+
+ @Test
+ public void readFromSocketWithDirectBuffer() throws IOException {
+ Socket socket = mock(Socket.class);
+ SocketChannel channel = mock(SocketChannel.class);
+ when(socket.getChannel()).thenReturn(channel);
+ final ByteBuffer buffer = ByteBuffer.allocateDirect(100); // non-heap
buffer
+ byte[] bytes = new byte[100];
+ InputStream stream = new ByteArrayInputStream(bytes);
+ when(channel.read(buffer)).thenAnswer((answer) -> {
+ buffer.put(bytes);
+ return buffer.position();
+ });
+ assertThat(buffer.hasArray()).isFalse();
+ SocketUtils.readFromSocket(socket, buffer, stream);
+ // the channel was used to read the bytes
+ verify(channel, times(1)).read(buffer);
+ // the buffer was filled
+ assertThat(buffer.position()).isEqualTo(bytes.length);
+ // the stream was not used
+ assertThat(stream.available()).isEqualTo(bytes.length);
+ }
+
+ @Test
+ public void readFromSSLSocketWithHeapBuffer() throws IOException {
+ Socket socket = mock(SSLSocket.class);
+ SocketChannel channel = mock(SocketChannel.class);
+ when(socket.getChannel()).thenReturn(channel);
+ final ByteBuffer buffer = ByteBuffer.allocate(100); // heap buffer
+ byte[] bytes = new byte[100];
+ InputStream stream = new ByteArrayInputStream(bytes);
+ assertThat(stream.available()).isEqualTo(bytes.length);
+ SocketUtils.readFromSocket(socket, buffer, stream);
+ // the channel was not used to read the bytes
+ verify(channel, times(0)).read(buffer);
+ // the buffer was filled
+ assertThat(buffer.position()).isEqualTo(bytes.length);
+ // the stream was used to read the bytes
+ assertThat(stream.available()).isZero();
+ }
+
+
+ @Test
+ public void readFromSSLSocketWithDirectBuffer() throws IOException {
+ Socket socket = mock(SSLSocket.class);
+ SocketChannel channel = mock(SocketChannel.class);
+ when(socket.getChannel()).thenReturn(channel);
+ final ByteBuffer buffer = ByteBuffer.allocateDirect(100); // non-heap
buffer
+ byte[] bytes = new byte[100];
+ InputStream stream = new ByteArrayInputStream(bytes);
+ assertThat(stream.available()).isEqualTo(bytes.length);
+ SocketUtils.readFromSocket(socket, buffer, stream);
+ // the channel was not used
+ verify(channel, times(0)).read(buffer);
+ // the buffer was filled
+ assertThat(buffer.position()).isEqualTo(bytes.length);
+ // the stream was used to fill the buffer
+ assertThat(stream.available()).isZero();
Review comment:
I recommend factoring common code out of these two test methods. In fact
all four methods have a common shape.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/net/SocketUtils.java
##########
@@ -89,6 +89,10 @@ public static boolean close(final ServerSocket serverSocket)
{
* and buffer.remaining is also zero the limit is changed to be
buffer.capacity
* before reading.
*
+ * @param socket the socket to read from
+ * @param inputBuffer the buffer into which data should be stored
+ * @param socketInputStream the socket's inputStream, included as a
parameter so it can be a
+ * buffered stream, if desired
Review comment:
"if desired" by whom—by what part of the code? The comment implies the
caller controls whether or not the stream is used, perhaps by making it `null`
or not. Reality is this parameter is dependent on the kind of object referenced
by the `socket` parameter.
Any time I see a dependency between parameters like this, especially when it
comes with a switch-on-type it makes me wonder if there is a mis-assignment of
responsibility somewhere. Is there already some object that could hold the
stream? If so, could we delegate this behavior to that object?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
