6
\$\begingroup\$

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 BufferedReader reader;
private Socket socket;
public boolean writing = true;
public boolean wantToRead = false;

public String getMessage() {
        synchronized (message) {
            while (writing) {
                try {
                    System.out.println("waiting");
                    wantToRead = true;
                    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 {
            while((c = reader.read()) != -1) {
                synchronized(message) {
                    message.append(Character.toString((char) c));
                    if(!reader.ready()) {
                        System.out.println("end of response");
                        if(wantToRead) {
                            writing = false;
                            wantToRead = false;
                            message.notify();
                        }
                    }
                }
            }
            System.out.println("zamykam socket");
            socket.close();
        } catch (IOException e) {
            e.printStackTrace();
        }
\$\endgroup\$
3
\$\begingroup\$
  • Instead of reading one character at a time, try the other BufferedReader methods that read many characters in one read.

  • 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<>();
    BufferedReader reader; // TODO init somewhere

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

    public void startReceivingMessages() {
        Executors.newSingleThreadExecutor().submit(() -> {
            try {
                String fullMessage = "";
                String line;
                while ((line = reader.readLine()) != null) {
                    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.

\$\endgroup\$
  • \$\begingroup\$ I need the code if(!reader.ready()) because i need to detect when the loop will block at reader.read(). My socket stream is almost always on so the loop will only end on crash. When connected and reading i'll be always in loop, and i need to know when the response ends. And reader.ready() can say me that the reader will block only on reader.read() so i cannot use readline() etc. \$\endgroup\$ – Gravian Jun 29 '14 at 18:40
  • \$\begingroup\$ OK, I understand your problem better now. You definitely want to use a BlockingQueue<String> which takes in sentences. \$\endgroup\$ – toto2 Jun 29 '14 at 22:36
  • \$\begingroup\$ I also added an item in my answer about writing and wantToRead. \$\endgroup\$ – toto2 Jun 29 '14 at 22:39
  • \$\begingroup\$ 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. \$\endgroup\$ – Gravian Jun 30 '14 at 21:44
  • \$\begingroup\$ 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. \$\endgroup\$ – toto2 Jun 30 '14 at 22:40
5
\$\begingroup\$

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
  }

Moreover, you barely need to use threads, use Executors instead

\$\endgroup\$
2
\$\begingroup\$
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.)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.