Understanding CompletableFuture simple implementation

Question

Consider the following code taken from this article. It implements something similar to a CompletableFuture for learning purposes.

Here's the get() function of WaitingFuture:

@Override
public V get() throws ExecutionException {
    this.thread = Thread.currentThread();
    LockSupport.park(this);

    if (throwable != null) {
        throw new ExecutionException(throwable);
    }

    return result;
}

And here's the run() function of RunnableWaitingFuture:

@Override
    public void run() {
            try {
                waitingFuture.result = userFunction.get();
            } catch (Throwable throwable) {
                waitingFuture.throwable = throwable;
            } finally {
                waitingFuture.finished = true;
                LockSupport.unpark(waitingFuture.thread);
            }
        }
    }

The Question:
It seems to me that if run() will finish before get() is even called then LockSupport.park(this); will be called after LockSupport.unpark(waitingFuture.thread), leaving the thread parking forever.

Is that true?

[The documentation for unpark](https://docs.oracle.com/en/java/javase/14/docs/api/java.base/java/util/concurrent/locks/LockSupport.html#unpark(java.lang.Thread)) says: “If the thread was blocked on park then it will unblock. Otherwise, its next call to park is guaranteed not to block.” So the only question is what the value of `waitingFuture.thread` is at the time `unpark` is called. — VGR, Aug 17 '20 at 17:35

Holger · Accepted Answer · 2020-08-17T16:13:00.420

4

park()/unpark() is different to wait/notify, as the signal won’t be lost if unpark has been called before park().

However, it’s still only a single bit that doesn’t count how often unpark has been called, so it’s still wrong to assume that all calls will be perfectly paired.

Further, park will silently return on interrupts and is even allowed to return spuriously, which means for no reason.

In other words, even returning from park() doesn’t guaranty that the condition has been fulfilled. Just like with the other notification mechanisms, there is no way around using it in a loop.

The cited code is even worse, as it has another race condition regarding the thread variable. There is no guaranty that it has been written at the point where the other thread reads it for notifying it.

edited Aug 17 '20 at 16:13

answered Aug 17 '20 at 16:06

Holger

285,553
42
434
765

I'm not sure I understand the first paragraph. It's like your'e saying the `unpark()` call presumably will cancel the `park()` call even though it happened before? – IsaacLevon Aug 17 '20 at 19:23
2

@yaseco [exactly](https://stackoverflow.com/questions/50370522/parking-threads-in-service/61149733#61149733) – Eugene Aug 17 '20 at 19:24
this spurious failures sort of sucks! I don't know the inner implementation or the reason that this might happen, but I can only assume that it is the same as spurious failures in `AtomicXXX::weakXXX` or the like. In those method that has to do with weak memory model platforms, where a "dirty" cache line might trigger a spurious failure. Do you happen to know what is going on here? I know this is too much to ask, I am just wondering. thank you. – Eugene Aug 17 '20 at 19:37
1

@Eugene I don’t see any reason to be annoyed. The other aspects do already lead to the requirement to use the construct in a pre-test loop. But when you do a pre-test, it’s possible to skip a `park` when you notice that the condition is fulfilled whereas a corresponding `unpark` has been performed already. Then, the next `park` will return immediately without the condition being fulfilled. Since you have to deal with it anyway (and the pre-test loop does deal with it), the actual spurious wakeup create no additional obstacle. But I remember someone mentioning the pthread lib as a reason. – Holger Aug 18 '20 at 07:08

Alexei Kaigorodov · Answer 2 · 2020-08-17T16:39:58.403

2

yes.

LockSupport.park(this);

should be replaced with something like

while (!waitingFuture.finished) {
    LockSupport.park(this);
}

Generally, LockSupport.park is too low a feature, and Object::wait or Condition::await should be used instead for reliability.

edited Aug 17 '20 at 16:39

answered Aug 17 '20 at 15:55

Alexei Kaigorodov

13,189
1
21
38

1

The issues of the question would also arise with `Object::wait` and `Condition::await`, i.e. they must be called in a loop with a pre-test. But, of course, they are preferable, as even if the code of the article was not broken, this usage allowed at most one thread calling `get`. Trying to implement support for multiple threads using `LockSupport` would boil down to reimplementing the existing `Lock`. – Holger Aug 17 '20 at 16:18

Understanding CompletableFuture simple implementation

2 Answers2