4
\$\begingroup\$

In my application I am attempting to queue outgoing messages by the following rules

  • By default they should be sent no less than messageDelay apart
  • Some messages can be sent immediately, completely bypassing all queued messages
  • Additionally, some immediately sent messages can reset the waiting time of queued messages

If this is confusing, this is what it would look like on the wire

|  Normal  |Immediate |Immediate-reset|
M----------M---M------M----M----------M ....

This was originally handled by a dedicated OutputThread and queue but for various reasons outside of the scope of this question I am attempting to implement this with ReentrantLock and Condition so when any of the methods finally returns, the message is known to of been sent (or an Exception is thrown meaning it didn't)

However this is my first time using ReentrantLock and while I think I've implemented it correctly I'm needing someone to verify that its correct since this is an extremely difficult thing to test

protected final ReentrantLock writeLock = new ReentrantLock(true);
protected final Condition writeNowCondition = writeLock.newCondition();

public void sendRawLine(String line) {
    //[Verify state code]
    writeLock.lock();
    try {
        sendRawLineToServer(line);
        //Block for messageDelay. If rawLineNow is called with resetDelay
        //the condition is tripped and we wait again
        while (writeNowCondition.await(getMessageDelay(), TimeUnit.MILLISECONDS)) {
        }
    } catch (Exception e) {
        throw new RuntimeException("Couldn't pause thread for message delay", e);
    } finally {
        writeLock.unlock();
    }
}

public void rawLineNow(String line, boolean resetDelay) {
    //[Verify state code]
    writeLock.lock();
    try {
        sendRawLineToServer(line);
        if (resetDelay)
            //Reset the 
            writeNowCondition.signalAll();
    } finally {
        writeLock.unlock();
    }
}

Is this the correct way to implement ReentrantLock based on my requirements above? Is there a better way (NOT USING THREADS) to do this?

\$\endgroup\$
3
  • \$\begingroup\$ Your code extracts only show the write side locking; how many readers are there? (otherwise, +1 for the recommended use of try/finally!) \$\endgroup\$
    – fge
    Commented Jun 19, 2013 at 20:53
  • \$\begingroup\$ @fge There is really only one single threaded reader. It just creates events and executes the listeners in a thread pool, some of which may have output \$\endgroup\$
    – TheLQ
    Commented Jun 20, 2013 at 1:48
  • \$\begingroup\$ In the first method, I wouldn't catch Exception... It is very broad, and do not forget that it also captures all RuntimeExceptions (therefore including NPE, etc) \$\endgroup\$
    – fge
    Commented Jun 20, 2013 at 4:43

1 Answer 1

3
\$\begingroup\$

I can see some problems with the code:

  • As @fge mentions, catching Exception is problematic. You should catch the exceptions that you are expecting to occur, and let any other unchecked exceptions propagate.

  • Wrapping the exceptions in RuntimeException is a bad idea unless it is really necessary. It is better to either let them propagate, or wrap them in a custom checked or unchecked exception.

  • The behaviour of the code doesn't match your description in one respect. You talk about queuing messages. In fact, messages are being sent immediately in all cases, and you are delaying after sending the message in the sendRawLine case.

  • In reality, you are blocking client threads rather using a queue data structure and a separate output thread. This might be the right things to do ... but it might be the wrong thing. It depends whether the client threads should be blocked until the their respective messages are sent. Either way, your characterization this as a "message queuing" problem is a bit misleading.


Re: #3) The idea was to delay the next message from sending. Otherwise all calls to sendRawLine would have a weird pause before doing anything. I'm looking into using the current time though to determine if waiting is even necessary.

First, that is NOT what your description of "message queuing" implies ... to me. And consider me as an examplar of someone else reading your code.

Second, (if I understand you correctly) using the current time to decide to whether to wait before a send would require that something keeps track of when each thread last sent a message.

Third, there is actually a good reason to not wait before sending a message. If you do wait before then, by the time the wait period has finished, the message could be rather out of date ... especially if there were a number of resets.

Finally, it strikes me that you may be over-using threads here and/or that it might be better for the entity (thread or FSM) responsible for generating messages should check whether a message needs to be sent before generating it.

\$\endgroup\$
1
  • \$\begingroup\$ #1 and #2) I will work on the exceptions. #3) The idea was to delay the next message from sending. Otherwise all calls to sendRawLine would have a weird pause before doing anything. I'm looking into using the current time though to determine if waiting is even necessary. #4) This was intentional. \$\endgroup\$
    – TheLQ
    Commented Jun 23, 2013 at 2:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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