I've written some code that is suppose to read socket all the time and when needed return the partial data from it. I just want the response to send a command. It works, but this is a bit messy implementation.

Is it possible to write it better?

public StringBuilder message = new StringBuilder();
private Socket socket;
public boolean writing = true;

public String getMessage() {
synchronized (message) {
while (writing) {
try {
System.out.println("waiting");
message.wait();
} catch (InterruptedException e) {
e.printStackTrace();
}
}
System.out.println("take the resource");
writing = true;
return message.toString().substring(0, message.toString().lastIndexOf("\n"));
}
}

@Override
public void run() {
int c;
try {
synchronized(message) {
message.append(Character.toString((char) c));
System.out.println("end of response");
writing = false;
message.notify();
}
}
}
}
System.out.println("zamykam socket");
socket.close();
} catch (IOException e) {
e.printStackTrace();
}


• Don't synchronize on actual useful objects since those objects might have some locks on them elsewhere, or internally. Either create some Object lock, or even better is to use a real lock from java.util.concurrent.locks.

• You don't need if(!reader.ready()). It's the exact same condition as the end of the while-loop. Just put that code after the while loop. You can synchronize again.

• writing and wantToRead should definitely be volatile since you use them in different threads. Also, you should probably rename them because it is not obvious what they are supposed to do.

• Actually, don't use either synchronized, wait/notify or locks. Those are primitive constructs from the early versions of Java. They have been replaced by the modern patterns in java.util.concurrent about a decade ago (see Java Concurrency in Practice (2006)). It's very hard to avoid (nearly undetectable) bugs when using the old constructs. You can look at BlockingQueue and Future as examples of the new concurrency classes.

• Don't use threads, but Executors (also in java.util.concurrent).

Here is a rewrite of your code with a BlockingQueue:

private static final String MESSAGE_SEPARATOR = ">";
private final BlockingQueue<String> blockingQueue = new LinkedBlockingQueue<>();

/**
* Blocking method.
*/
public String takeNextMessage() throws InterruptedException {
return blockingQueue.take();
}

public void startReceivingMessages() {
try {
String fullMessage = "";
String line;
if (line.startsWith(MESSAGE_SEPARATOR)) {
if (!fullMessage.isEmpty())
blockingQueue.put(fullMessage);
fullMessage = "";
}
fullMessage += line;
}
if (!fullMessage.isEmpty())
blockingQueue.put(fullMessage);
} catch (Exception e) {
e.printStackTrace();
}
});
}


I hope you can appreciate how hugely simpler this is. It's not just that it is shorter, but that there is absolutely no reasoning to do about which variable in which thread has to be synchronized, and how and where does variables, or code blocks, should synchronize. With the modern constructs, there is no need to use synchronized, wait/notify, volatile, or xxxx.lock() in the vast majority of problems. The code is much much easier to read and reason about.

• OK, I understand your problem better now. You definitely want to use a BlockingQueue<String> which takes in sentences. – toto2 Jun 29 '14 at 22:36
• I also added an item in my answer about writing and wantToRead. – toto2 Jun 29 '14 at 22:39
• That's great but still i need to read multiline resposnses and know when there is the end of message. I pass the command to the socket, need to wait for reader to read the response and then take it. As you can see i update a question with my rewroted code. I put to the queue only if the reader read the full message, while take waits for it. I'm also not sure about executor i use. – Gravian Jun 30 '14 at 21:44
• From what I understand from the doc, !in.ready() is equivalent to in.read() == -1. So it looks to me that you will read a single message. Also, I would not make the queue of capacity 1, since you can than accumulate messages without blocking. – toto2 Jun 30 '14 at 22:40

You are locking on a public field message which might cause a deadlock. If locking is needed, then do it on a private field.

private final Object SYNC_OBJECT = new Object();
...
synchronized(SYNC_OBJECT){
}


Your class extends Thread apparently, and the rule is : favor composition over inheritance.

  private Runnable runnable;

public void startListening(){
// use the runnable
}


return message.toString().substring(0, message.toString().lastIndexOf("\n"));


message is a StringBuilder and StringBuilder has both substring(beginIndex, endIndex) and lastIndexOf(str) as well, so you could omit the toString() calls. The following is the same:

return message.substring(0, message.lastIndexOf("\n"));


Furthermore, I'd create an explanatory local variable for the result to explain what is its purpose and make reading/maintenance easier:

String messageBeforeLastLineBreak = message.substring(0, message.lastIndexOf("\n"));
return messageBeforeLastLineBreak;


(Clean Code by Robert C. Martin, G19: Use Explanatory Variables; Refactoring: Improving the Design of Existing Code by Martin Fowler, Introduce Explaining Variable)

Be aware, that lastIndexOf() could return -1 when there is no \n in the message and you'll get a StringIndexOutOfBoundsException. I guess you should handle that case too. (If it's an error of the client throw an exception which says that this is a client error, it would help debugging.)