2

I'm using the observer pattern and a BlockingQueue to add some instances. Now in another method I'm using the queue, but it seems take() is waiting forever, even though I'm doing it like this:

/** {@inheritDoc} */
@Override
public void diffListener(final EDiff paramDiff, final IStructuralItem paramNewNode,
    final IStructuralItem paramOldNode, final DiffDepth paramDepth) {
    final Diff diff =
        new Diff(paramDiff, paramNewNode.getNodeKey(), paramOldNode.getNodeKey(), paramDepth);
    mDiffs.add(diff);
    try {
        mDiffQueue.put(diff);
    } catch (final InterruptedException e) {
        LOGWRAPPER.error(e.getMessage(), e);
    }
    mEntries++;

    if (mEntries == AFTER_COUNT_DIFFS) {
        try {
            mRunner.run(new PopulateDatabase(mDiffDatabase, mDiffs));
        } catch (final Exception e) {
            LOGWRAPPER.error(e.getMessage(), e);
        }
        mEntries = 0;
        mDiffs = new LinkedList<>();
    }
}

/** {@inheritDoc} */
@Override
public void diffDone() {
    try {
        mRunner.run(new PopulateDatabase(mDiffDatabase, mDiffs));
    } catch (final Exception e) {
        LOGWRAPPER.error(e.getMessage(), e);
    }
    mDone = true;
}

whereas mDiffQueue is a LinkedBlockingQueue and I'm using it like this:

while (!(mDiffQueue.isEmpty() && mDone) || mDiffQueue.take().getDiff() == EDiff.INSERTED) {}

But I think the first expression is checked whereas mDone isn't true, then maybe mDone is set to true (an observer always is multithreaded?), but it's already invoking mDiffQueue.take()? :-/

Edit: I really don't get it right now. I've recently changed it to:

synchronized (mDiffQueue) {
    while (!(mDiffQueue.isEmpty() && mDone)) {
        if (mDiffQueue.take().getDiff() != EDiff.INSERTED) {
            break;
        }
    }
}

If I wait in the debugger a little time it works, but it should also work in "real time" since mDone is initialized to false and therefore the while-condition should be true and the body should be executed.

If the mDiffQueue is empty and mDone is true it should skip the body of the while-loop (which means the queue isn't filled anymore).

Edit: Seems it is:

synchronized (mDiffQueue) {
    while (!(mDiffQueue.isEmpty() && mDone)) {
         if (mDiffQueue.peek() != null) {
             if (mDiffQueue.take().getDiff() != EDiff.INSERTED) {
                 break;
             }
         }
    }
}

Even though I don't get why the peek() is mandatory.

Edit:

What I'm doing is iterating over a tree and I want to skip all INSERTED nodes:

for (final AbsAxis axis = new DescendantAxis(paramRtx, true); axis.hasNext(); axis.next()) {
    skipInserts();
    final IStructuralItem node = paramRtx.getStructuralNode();
    if (node.hasFirstChild()) {
        depth++;
        skipInserts();
        ...

Basically computing the maximum depth or level in the tree without considering nodes which have been deleted in another revision of the tree (for a comparsion Sunburst visualization), but ok, that's maybe out of scope. Just to illustrate that I'm doing something with nodes which haven't been inserted, even if it's just adjusting the maximum depth.

regards,

Johannes

Johannes
  • 2,021
  • 2
  • 23
  • 40

2 Answers2

2

take() is a "blocking call". That means it will block (wait forever) until something is on the queue then it will return what was added. Of course, if something is on the queue, it will return immediately.

You can use peek() to return what would be returned by take() - that is, peek() returns the next item without removing it from the queue, or returns null if there's nothing on the queue. Try using peek() instead in your test (but check for null too).

Bohemian
  • 412,405
  • 93
  • 575
  • 722
  • Yeah that's why I introduced the boolean member variable which is set to true when no more diffs are encountered, and as such the BlockingQueue isn't filled anymore, then it should skip the while-body. See my edit of the original post :-/. I can't see how peek() would help, since sometimes the queue might be currently empty but may be filled afterwards. – Johannes Sep 07 '11 at 12:30
1

First advice: don't synchronized (mDiffQueue). You would get deadlock if the LinkedBlockingQueue had some synchronized method; it's not the case here, but it's a practice that you should avoid. Anyway, I don't see why you are synchronizing at that point.

You have to "wake up" periodically while waiting to check if mDone has been set:

while (!(mDiffQueue.isEmpty()  && mDone)) {
   // poll returns null if nothing is added in the queue for 0.1 second.
   Diff diff = mDiffQueue.poll(0.1, TimeUnit.SECONDS); 
   if (diff != null)
      process(diff);
}

This is about the same as using peek, but peek basically waits for a nanosecond instead. Using peek is called "busy waiting" (your thread runs the while loop non-stop) and using pool is called "semi-busy waiting" (you let the thread sleep at intervals).

I guess in your case process(diff) would be to get out of the loop if diff is not of type EDiff.INSERTED. I'm not sure if that is what you are trying to accomplish. This seems odd since you are basically just stalling the consumer thread until you get a single element of the right type, and then you do nothing with it. And you cannot receive the future incoming elements since you are out of the while loop.

toto2
  • 5,306
  • 21
  • 24
  • My diffListener-method is called from another thread, so I didn't know if it's threadsafe and just wanted to synchronize the call. So I safely can remove it? But ok, I assume take() and put() are threadsafe. I needed something to check that the mDiffQueue isn't filled anymore (for the last node). Otherwise take() would block forever. – Johannes Sep 07 '11 at 12:53
  • Yes, you can remove it. LinkedBlockingQueue is well synchronized by itself. – toto2 Sep 07 '11 at 12:54
  • I don't understand the last part of your paragraph. I guess I actually don't understand what you are trying to do. It's not clear from your original post. Who's putting Diff's in the queue and who's getting them? And doing what with them? – toto2 Sep 07 '11 at 12:59
  • Mh, the observerable class stops sending, then it calls diffDone() and therefore I had to check if the BlockingQueue isn't filled anymore (with mDone, which is set to true at the end). – Johannes Sep 07 '11 at 13:09
  • They are putted into the queue in `public void diffListener(final EDiff paramDiff, final IStructuralItem paramNewNode, final IStructuralItem paramOldNode, final DiffDepth paramDepth) {...}` – Johannes Sep 07 '11 at 13:11
  • I think I get it. You have one producing thread and one consuming thread and you have a `volatile boolean` flag `mDone`. When the producer knows that it will not produce new Diff's it sets `mDone = true`. The consumer thread processes incoming messages in an infinite while loop, except that it stops waiting if it sees `mDone == true`. – toto2 Sep 07 '11 at 13:17
  • Yes, almost... it's not an infinite loop, I just want to skip `EDiff.INSERTED` nodes ;-) but other than that, yes. Well, what I don't get is why I had to use `if (mDiffQueue.peek() != null) {` which works, see my latest edit. – Johannes Sep 07 '11 at 13:28
  • I completely changed my answer. I still don't get what you want to do with `EDiff.INSERTED`. My last paragraph is about what your code is doing now for `EDiff.INSERTED`, but I don't think it does what you want. You are aware that `break` gets out of the `while` loop, not only the `if`? – toto2 Sep 07 '11 at 13:42
  • Yes, maybe my latest edit describes what I'm doing a little bit. I'm just going to speed up everything in my application/visualization a little bit. – Johannes Sep 07 '11 at 13:49
  • OK. I'm still not sure what an "inserted" node is. But are you sure that everything is well ordered, with all "inserted" nodes added in the queue first, with no interleaving with non-inserted nodes? If you have many consumer threads and they are "inserting" nodes in parallel then the synchronization from LinkedBlockingQueue is insufficient. – toto2 Sep 07 '11 at 13:55
  • Just one producer thread, which is a diff-algorithm to compute diffs between two tree-revisions (and between two nodes each, such that I get an enum with the kind of diff, INSERTED, DELETED, UPDATED, REPLACED, SAME). It already works, but now I'm going to replace a Future list with descendants for each node also with an observer/BlockingQueue, such that everything which is possible works in a pipelined way. – Johannes Sep 07 '11 at 14:02
  • BTW: It wasn't sufficient to use a poll() with a timeout of 100ms, even with 500ms it isn't sufficient. I have reinserted the peek(), but hm... – Johannes Sep 07 '11 at 14:05
  • It shouldn't make a difference what time interval you use (or use peek which is a 0 time interval). Maybe you have another thread that's consuming from the queue too? Or you have some other synchronization problem. You should probably not just ignore this problem; it might come back later, or if you run your code on a different machine. – toto2 Sep 07 '11 at 14:34
  • OK, I thought of one bug: peek does not remove the element from the queue, so you do a `take` after. But poll does remove the element from the list, so if you still `take` after a poll, you are actually waiting for a new element. My code is different from yours, so check that again if you change peek by poll. – toto2 Sep 07 '11 at 14:41
  • I'm not reading from the BlockingQueue other than the thread meantioned (the same thread which puts Diffs into the queue). Hm, it's really strange. I have deleted the `peek()` and replaced `take()` with `poll()` but it makes the difference... I mean it's somehow not equivalent. With peek() and take() it seems to work, but with poll() it doesn't, strange enough. – Johannes Sep 07 '11 at 14:42
  • Funny... my post 1 minute __before__ your observation about peek/poll explains it. – toto2 Sep 07 '11 at 15:08