0

I'm using SocketChannel for single connection like this:

int sampleBufferSize = 50;
StringBuilder data = new StringBuilder();
ByteBuffer bf = ByteBuffer.allocate(sampleBufferSize);
SocketChannel sc = new SocketChannel();
while(true)
    if(sc.read(bf) > 0){
        bf.flip();
        while(bf.hasRemaining())
            data.append((char) bf.get());
        bf.clear();
    }else{
        fireDataReceived(data.toString());
        data.delete(0, data.length());
    }

This code is not very efficient, but it reads HTTP POST request 130 KB from the same PC in 0.05 seconds. Now I'm trying to write a class with similar functionality but using Socket. Here is the code:

private static final int TIMEOUT_MILLIS = 50;
private boolean reading = false;
private long readBeginTime = 0;
private StringBuilder buffer = new StringBuilder();
private Thread thread = new Thread(){
    public void run(){
        while(!isInterrupted()){
            try {
                int b = getInputStream().read();
                if(b == -1){
                    if(reading)
                        fireDataReceived();
                    close();
                }else{
                    if(!reading){
                        reading = true;
                        readBeginTime = System.currentTimeMillis();
                        setSoTimeout(TIMEOUT_MILLIS);
                    }
                    buffer.append((char) b);
                }
            } catch (SocketTimeoutException e){
                fireDataReceived();
            } catch (IOException e) {
                e.printStackTrace();
                if(reading)
                    fireDataReceived();
                close();
            }
        }
    }
};
private void fireDataReceived(){
    BufferedSocketEvent e = new BufferedSocketEvent(this, System.currentTimeMillis() - readBeginTime, buffer.toString());
    buffer.setLength(0);
    reading = false;
    try {
        setSoTimeout(0);
    } catch (SocketException e1) {
        e1.printStackTrace();
    }
    for(BufferedSocketListener listener: listeners)
        listener.dataReceived(e);
}

And the problem is that it takes 0.4 seconds for the same request and I have no idea why does it take so long. Please, explain what is wrong with my code.

Anton Onipko
  • 108
  • 6

2 Answers2

0

The problem with your streams code is that you're reading a byte at a time, which is slow, and also appending to the StringBuilder one at a time, ditto. Try this:

BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream());
char[] chars = new char[8192];
int count;
while ((count = in.read(chars)) > 0)
{
    buffer.append(chars, 0, count);
    fireDataReceived();
}

Note that it isn't correct to use read timeouts as a means to separate requests. You need to parse the data to do that. TCP is a streaming protocol without message boundaries, and there are no guarantees about separate sends being received separately.

user207421
  • 305,947
  • 44
  • 307
  • 483
  • BufferedReader is a good solution, but if I make wrong request (without line feed/carriage return symbols at all), I won't receive those data. – Anton Onipko Dec 19 '14 at 22:33
  • And talking about fireDataReceived in SocketTimeoutException catch clause, such exception can be thrown only when socket timeout is greater then 0. And timeout is set to TIMEOUT_MILLIS only when first byte of data received. This means SocketTimeoutException clause can't be reached when there is no data already buffered. – Anton Onipko Dec 19 '14 at 22:36
  • @AntonOnipko You are mistaken. This code does not rely on carriage returns or line feeds, as it does not call `readLine()`. – user207421 Dec 19 '14 at 22:41
  • Yes, my mistake. Will try now. – Anton Onipko Dec 19 '14 at 22:44
0

You have to use readLine() or similar method. End of HTTP header is marked by empty line. If you don't detect lines you cannot know when to stop reading data. Relying on timeouts or TCP fragments is not a correct solution. If request doesn't contain new line, you need to wait until one appears or the connection is terminated/timed out. You should wait at least a couple of seconds.

I would also get rid of those listeners. Being able to add multiple listeners for single socket might seem like a good idea but the only sensible thing to do with HTTP header is to parse it. The parser also needs to inform the reader when to stop reading.

I would start with something like this:

ServerSocket serverSocket = new ServerSocket(8888);
Socket clientSocket = serverSocket.accept();
BufferedReader reader = new BufferedReader(new InputStreamReader(clientSocket.getInputStream(), "ASCII"));
String[] r = reader.readLine().split(" ");
String type = r[0];
String resource = r[1];
String version = r[2];
Map<String, String> headers = new HashMap<String,String>();
while(true) {
    String line = reader.readLine();
    if(line.isEmpty()) {
        break;
    }
    String headerLine[] = line.split(":",2);
    headers.put(headerLine[0],headerLine[1].trim());
}
processHeader(type, resource, version, headers);
Piotr Praszmo
  • 17,928
  • 1
  • 57
  • 65
  • `readLine()` would be a good choice if everyone followed the standarts and there was no malevolent people. My question started pretty simply: once I was creating an app with web interface and found out that there is no line ending after HTTP POST's body. And then I thought "maybe a client can send wrong Content-length header and I won't be able to read the whole body or I would stuck waiting for unexisting rest data". And then I thought "OMG, anyone can make no-line-ending request and it will "hang"". That's why I need to read some minimally required data regardless of format. – Anton Onipko Dec 20 '14 at 00:41
  • If you are worried about hanged connections, you should set timeout to a couple of seconds and just drop slow connections. Apache does it like this (with 60 second timeout by default). In your current approach every client has to wait the same amount of time no mater if it's LAN or dial-up. Increasing the timeout to make slow clients work will reduce the performance of fast clients. It just cannot work fast. – Piotr Praszmo Dec 20 '14 at 01:04
  • There doesn't have to be a line ending after HTTP POST's body. This answer only concerns the header. Once you've read the header as per this answer you should use `read()` as per my answer. – user207421 Dec 20 '14 at 07:17