From 0da10958ce02b8d7ae8a5d9b5a7ec249203b2441 Mon Sep 17 00:00:00 2001 From: Nick Hudson Date: Wed, 9 Oct 2013 11:40:12 +0100 Subject: Revert "synchronized" patch and fix use of ByteArray inside JavaConnection Some discussion followed the "Fix synchronization problem in ByteArray" patch, and that led us to believe that it would be better to change the JavaConnection class so that it does not rely on being able to pass ByteArrays around in a way that makes them vulnerable to the problems that had been seen. The JavaConnection class accepts a ByteArray in its "write()" method, and emits a ByteArray when it has read data. ByteArrays are not the ideal way for the JavaConnection class to manipulate data and so this patch changes the implementation so that: a) the "write()" method extracts the byte[] from the supplied ByteArray and uses these objects, rather than keeping references to the ByteArray objects (which might lead to synchronisation issues). b) the "doRead()" method uses a ByteArrayOutputStream to hold incoming data, and only constructs a ByteArray out of it when it is ready to return the data to the application. These changes make the class more efficient, since in the case of (a), the need to create temporary ByteArrays is removed, and in (b) the code no longer creates ByteArrays by iterating through the network data one byte at a time and appending it to a ByteArray. It also means that the "synchronized" patch (which would fix the problem) is no longer necessary, and so that code is reverted. Test-information: I patched the code to emulate the situation that would occur when a buffer is only partially written, and verified that in this case it correctly re-inserted the unwritten portion of the buffer at the front of the pending queue. Ran MLC to various servers, all seems to work OK. Tested in Harrier, seems to work OK, and does not exhibit problems that we had seen previously which led us to investigate this issue. Change-Id: Ifcda547402430c87da45ba7d692518b5af285763 Signed-off-by: Nick Hudson diff --git a/src/com/isode/stroke/base/ByteArray.java b/src/com/isode/stroke/base/ByteArray.java index 22d81b7..d3942ce 100644 --- a/src/com/isode/stroke/base/ByteArray.java +++ b/src/com/isode/stroke/base/ByteArray.java @@ -77,7 +77,7 @@ public class ByteArray { * @return array copy of internal data, will never be null, but may * contain zero elements. */ - public synchronized byte[] getData() { + public byte[] getData() { if (dataCopy_ == null) { dataCopy_ = new byte[getSize()]; for (int i = 0; i < data_.size(); i++) { @@ -167,7 +167,7 @@ public class ByteArray { * @param b a single byte * @return a reference to the updated object */ - public synchronized ByteArray append(byte b) { + public ByteArray append(byte b) { dataCopy_ = null; /* Invalidate cache */ data_.add(Byte.valueOf(b)); return this; @@ -278,7 +278,7 @@ public class ByteArray { /** * Clears the contents of this ByteArray, leaving it with zero elements. */ - public synchronized void clear() { + public void clear() { data_ = new Vector(); dataCopy_ = null; } diff --git a/src/com/isode/stroke/network/JavaConnection.java b/src/com/isode/stroke/network/JavaConnection.java index 31b07a0..3f768dc 100644 --- a/src/com/isode/stroke/network/JavaConnection.java +++ b/src/com/isode/stroke/network/JavaConnection.java @@ -8,6 +8,7 @@ */ package com.isode.stroke.network; +import java.io.ByteArrayOutputStream; import java.io.IOException; import java.net.InetSocketAddress; import java.net.Socket; @@ -16,6 +17,7 @@ import java.nio.channels.SelectionKey; import java.nio.channels.Selector; import java.nio.channels.SocketChannel; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -29,7 +31,7 @@ public class JavaConnection extends Connection implements EventOwner { private class Worker implements Runnable { private final HostAddressPort address_; - private final List writeBuffer_ = Collections.synchronizedList(new ArrayList()); + private final List writeBuffer_ = Collections.synchronizedList(new ArrayList()); public Worker(HostAddressPort address) { address_ = address; @@ -156,8 +158,7 @@ public class JavaConnection extends Connection implements EventOwner { return; } - ByteArray data = writeBuffer_.get(0); - byte[] bytes = data.getData(); + byte[] bytes = writeBuffer_.get(0); int bytesToWrite = bytes.length; if (bytesToWrite == 0) { @@ -167,8 +168,8 @@ public class JavaConnection extends Connection implements EventOwner { writeBuffer_.remove(0); return; } + ByteBuffer byteBuffer = ByteBuffer.wrap(bytes); - /* * Because the SocketChannel is non-blocking, we have to * be prepared to cope with the write operation not @@ -190,15 +191,13 @@ public class JavaConnection extends Connection implements EventOwner { } /* The buffer was *partly* written. This means we have to - * remove that part. We do this by creating a new ByteArray + * remove that part. We do this by creating a new byte[] * with the remaining bytes in, and replacing the first * element in the list with that. */ byte[] remainingBytes = new byte[bytesToWrite - bytesWritten]; - System.arraycopy(bytes, bytesWritten,remainingBytes,0, remainingBytes.length); - ByteArray leftOver = new ByteArray(remainingBytes); - - writeBuffer_.set(0, leftOver); + remainingBytes = Arrays.copyOfRange(bytes, bytesWritten, bytes.length); + writeBuffer_.set(0, remainingBytes); return; } @@ -210,21 +209,21 @@ public class JavaConnection extends Connection implements EventOwner { private ByteArray doRead() throws IOException { ByteBuffer byteBuffer = ByteBuffer.allocate(1024); - ByteArray data = new ByteArray(); int count = socketChannel_.read(byteBuffer); if (count == 0) { - return data; + return new ByteArray(); } + + ByteArrayOutputStream byteArrayOutputStream = + new ByteArrayOutputStream(1024); + while (count > 0) { byteBuffer.flip(); byte[] result = new byte[byteBuffer.remaining()]; byteBuffer.get(result); byteBuffer.compact(); - for (int i=0; i