2
\$\begingroup\$

I have implemented a class that has a method to wait until "something else" has happened. When this has happened, the current thread stops waiting and returns from the method. This is my current code:

public class InMemoryAviReadyNotifier implements AviReadyNotifier, MessageListener<UrlAviMessage>
{
    private Map<SourceAndFileName, CountDownLatch> m_map = Maps.newConcurrentMap();
    private Map<SourceAndFileName, URL> m_resultMap = Maps.newConcurrentMap();

    @Override
    public void messageReceived( MessageSource messageSource, UrlAviMessage message )
    {
        SourceAndFileName key = new SourceAndFileName( (DetectorMessageSource)messageSource, message.getFileName() );
        CountDownLatch countDownLatch = m_map.get( key );
        if( countDownLatch != null )
        {
            m_resultMap.put( key, message.getLocation() );
            countDownLatch.countDown();
        }
        m_map.remove( key );
    }

    @Override
    public Class<? extends DeviceToServerMessage> responseClass()
    {
        return UrlAviMessage.class;
    }

    @Override
    public InputStream waitForAvi( DetectorMessageSource messageSource, String aviFileName ) throws InterruptedException
    {
        CountDownLatch countDownLatch = new CountDownLatch( 1 );
        SourceAndFileName key = new SourceAndFileName( messageSource, aviFileName );
        m_map.put( key, countDownLatch );
        countDownLatch.await();
        URL url = m_resultMap.get( key );
        // TODO download the avi to a temp local folder so we can add connect and read timeouts on the connection
        return url.openStream();
    }

    private static class SourceAndFileName
    {
        private final DetectorMessageSource m_source;
        private final String m_fileName;

        public SourceAndFileName( DetectorMessageSource source, String fileName )
        {
            m_source = source;
            m_fileName = fileName;
        }

        @Override
        public boolean equals( Object o )
        {
            boolean result = false;

            if (this == o)
            {
                result = true;
            }
            else if (o instanceof SourceAndFileName)
            {
                SourceAndFileName other = (SourceAndFileName)o;

                result = Objects.equals( m_source, other.m_source)
                        && Objects.equals( m_fileName, other.m_fileName );
            }

            return result;
        }

        @Override
        public int hashCode()
        {
            return Objects.hash( m_source, m_fileName );
        }

        @Override
        public String toString()
        {
            return toStringHelper( this )
                    .add( "source", m_source )
                    .add( "fileName", m_fileName )
                    .toString();
        }
    }
}

The waitForAvi method gets called from one thread. It will wait until messageReceived gets called with the good arguments and then it will countdown the CountDownLatch. I need to get the message.getLocation() return value (an URL) in the waitForAvi method. I using 2 Maps like I do here the best option or are there alternatives I could consider?

I am aware that in the current implementation I can potentially wait forever if the messageReceived never happens, but adding timeouts to this code has not been done yet.

\$\endgroup\$
1
  • \$\begingroup\$ I personally tend to separate thread logics (tasks) in different classes, and make them communicate using message queues. Each task has an input queue which it can poll for new messages ad lib, and it may also post messages to other tasks input queues. \$\endgroup\$
    – didierc
    Oct 24, 2014 at 13:58

1 Answer 1

1
\$\begingroup\$

You have an interesting problem to solve. Your solution would be helped a lot with a small extension to the SourceAndFileName class. Consider the following version of it:

private static class SourceAndFileName
{
    private final DetectorMessageSource m_source;
    private final String m_fileName;

    private final ReentrantLock lock = new ReentrantLock();
    private final Condition arrived = lock.condition();
    private URL aviURL = null;

    public SourceAndFileName( DetectorMessageSource source, String fileName )
    {
        m_source = source;
        m_fileName = fileName;
    }

    public void setURL(URL url) {
        lock.lock();
        try {
            if (aviURL == null) {
                aviURL = url;
            }
            arrived.signalAll();
        } finally {
            lock.unlock();
        }
    }

    public URL getURL(long timeout) {
        long now = System.currentTimeMillis();
        final long delayto = now + timeout;
        lock.lock();
        try {
            while (aviURL == null && now < delayto) {
                arrived.await(delayto - now, TimeUnit.MILLISECONDS);
                now = System.currentTimeMillis();
            }
            return aviURL
        } finally {
            lock.unlock();
        }

    }

    .... the rest is the same.....
}

OK, so, with the above class, you can get (with a timed delay, if you want) the URL, and you can set it too. This encapsulates the logic for the process well... but, the problem is that you need to have one, and only one instance of this for each source/fileName. This is solved with a single ConcurrentMap... (using the same key/value instances).

private ConcurrentMap<SourceAndFileName, SourceAndFileName> m_map = Maps.newConcurrentMap();

private SourceAndFileName getKey(DetectorMessageSource messageSource, String fileName)
{
    // be optimistic, and assume we are the first in....
    SourceAndFileName key = new SourceAndFileName(messageSource, fileName);
    SourceAndFileName current = m_map.putIfAbsent(key,key);
    if (current != null) {
        // we were wrong, and not the first in, so use the key from the previous one.
        return current;
    }
    return key;
}

@Override
public void messageReceived( MessageSource messageSource, UrlAviMessage message )
{
    // be optimistic, and assume we are first in....
    SourceAndFileName key = getKey((DetectorMessageSource)messageSource, message.getFileName());

    // set the location, which will trigger the get to return, if necessary...
    key.setURL(message.getLocation());

}

@Override
public InputStream waitForAvi( DetectorMessageSource messageSource, String aviFileName ) throws InterruptedException
{
    // be optimistic, and assume we are first in....
    SourceAndFileName key = getKey((DetectorMessageSource)messageSource, message.getFileName());

    URL url = key.get( 5000 ); // 5 second delay....

    // TODO download the avi to a temp local folder so we can add connect and read timeouts on the connection

    return url.openStream();
}

A more general code review is also in order...

  • The m_... type prefixing of variables is not useful in modern IDE's where it is normally color-coded where variables are from....
  • Java style guidelines put the { opening brace on the same line as the block condition (while/if/etc.).
  • your hashCode/equals methods on the key class would benefit from early-return statements instead of carrying the result variable.
\$\endgroup\$
4
  • \$\begingroup\$ m_.. and placing of the opening brace is part of the codestyle guidelines we have. \$\endgroup\$ Oct 24, 2014 at 14:25
  • \$\begingroup\$ Let us continue this discussion in chat. \$\endgroup\$ Oct 24, 2014 at 14:41
  • \$\begingroup\$ Last question: why the checking of the time ? Why not just put the timeout in await() ? \$\endgroup\$ Oct 24, 2014 at 14:43
  • \$\begingroup\$ all await-type operations can occasionaly be victims of 'spurrious wakeups'. They are horrible bugs to track down. See Do Spurrious wakeups actually happen \$\endgroup\$
    – rolfl
    Oct 24, 2014 at 14:49

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.