Swiftdiff options
| author | Nick Hudson <nick.hudson@isode.com> | 2013-10-09 10:40:12 (GMT) |
|---|---|---|
| committer | Nick Hudson <nick.hudson@isode.com> | 2013-10-15 15:28:53 (GMT) |
| commit | 0da10958ce02b8d7ae8a5d9b5a7ec249203b2441 (patch) | |
| tree | 8f812f06c3e3a586ad1c728a63c22679f02f1a4e | |
| parent | 5cef6746d1997e4ecc9ad2c69901b69128a4d958 (diff) | |
| download | stroke-0da10958ce02b8d7ae8a5d9b5a7ec249203b2441.zip stroke-0da10958ce02b8d7ae8a5d9b5a7ec249203b2441.tar.bz2 | |
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 <nick.hudson@isode.com>
| -rw-r--r-- | src/com/isode/stroke/base/ByteArray.java | 6 | ||||
| -rw-r--r-- | src/com/isode/stroke/network/JavaConnection.java | 36 |
2 files changed, 22 insertions, 20 deletions
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<Byte>(); 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<ByteArray> writeBuffer_ = Collections.synchronizedList(new ArrayList<ByteArray>()); + private final List<byte[]> writeBuffer_ = Collections.synchronizedList(new ArrayList<byte[]>()); 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<result.length; i++) { - data.append(result[i]); - } - + byteArrayOutputStream.write(result); count = socketChannel_.read(byteBuffer); } if (count == -1) { @@ -233,7 +232,10 @@ public class JavaConnection extends Connection implements EventOwner { */ throw new IOException("socketChannel_.read returned -1"); } - return data; + + /* There is no need to close the ByteArrayOutputStream */ + return new ByteArray(byteArrayOutputStream.toByteArray()); + } private void handleConnected(final boolean error) { @@ -299,7 +301,7 @@ public class JavaConnection extends Connection implements EventOwner { @Override public void write(ByteArray data) { - worker_.writeBuffer_.add(data); + worker_.writeBuffer_.add(data.getData()); if (selector_ != null) { selector_.wakeup(); } |