Synchronize evaluateGroupCached to avoid concurrent access to cache

[lefou]

With this change, we synchronize evaluations of the same terminal task. This isn't necessarily needed for normal Mill executions, but in a BSP context, where multiple requests where handled concurrently in the same Mill instance, evaluating the same task concurrently can happen.

We don't synchronize multiple Mill-instances (e.g. run in two shells) or multiple evaluator-instances (which should have different out-dirs anyway).

Fix: #2818

Pull request: #2980

[lihaoyi]

Does this need to be a while? I thought we're inside lockKeys.synchronized, there should be no need to "try again" since nobody else else going to be able to mutate lockKeys

[lefou]

I think yes. At least, the Javadoc of the Object.wait method suggests to do so:

As in the one argument version, interrupts and spurious wakeups are possible, and this method should always be used in a loop:

synchronized (obj) {
    while (<condition does not hold>)
        obj.wait();
    ... // Perform action appropriate to condition
}

[lihaoyi]

Using a file lock to allow mutex between different Mill processes would definitely also be nice. e.g. It's not uncommon for IntelliJ to be running a Mill process and for my shell to be running a Mill process. But intra-process mutex is already an improvement to the status quo so we can go ahead with this first

[lefou]

Using a file lock to allow mutex between different Mill processes would definitely also be nice. e.g. It's not uncommon for IntelliJ to be running a Mill process and for my shell to be running a Mill process. But intra-process mutex is already an improvement to the status quo so we can go ahead with this first

I think providing a file-based lock strategy is the next logical step, but as it still needs to be performed on a task-granualarity, it might come with some performance implications. Maybe, we want it to be configurable. Or just use it, if we detect, that there is another Mill running.