summaryrefslogtreecommitdiffstats
path: root/src
diff options
context:
space:
mode:
authorNick Hudson <nick.hudson@isode.com>2013-10-09 10:40:12 (GMT)
committerNick Hudson <nick.hudson@isode.com>2013-10-15 15:28:53 (GMT)
commit0da10958ce02b8d7ae8a5d9b5a7ec249203b2441 (patch)
tree8f812f06c3e3a586ad1c728a63c22679f02f1a4e /src
parent5cef6746d1997e4ecc9ad2c69901b69128a4d958 (diff)
downloadstroke-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>
Diffstat (limited to 'src')
-rw-r--r--src/com/isode/stroke/base/ByteArray.java6
-rw-r--r--src/com/isode/stroke/network/JavaConnection.java36
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();
}