Fix potential race in Spawn#getLocalResources()

[fmeum]

Both implementations performed two reads of the nullable instance variable, which could be reordered so that the first read returns a non-null value and the second read returns null.

[fmeum]

CI failure appears to be unrelated.

[meisterT]

I am fine with the code change, but do not follow how a race can happen here. Can you please elaborate?

[justinhorvitz]

I am fine with the code change, but do not follow how a race can happen here. Can you please elaborate?

Agree, I don't think reordering in manner suggested is possible.

[fmeum]

I am fine with the code change, but do not follow how a race can happen here. Can you please elaborate?

I may be missing something, so please doublecheck: Looking at

  private ResourceSet localResourcesCached;

  ...

  public ResourceSet getLocalResources() throws ExecException {
    if (localResourcesCached == null) {
      // Not expected to be called concurrently, and an idempotent computation if it is.
      localResourcesCached = localResourcesSupplier.get();
    }
    return localResourcesCached;
  }

the local variable isn't volatile, so writes to it (and thus reads from it) are not ordered. The getLocalResources function reads from it two times (first line and last line) and could get back null both times, following this example. I can't get this to happen on my Apple Silicon MacBook in JCStress, but I don't think it's safe as per the JMM.

[justinhorvitz]

I'm not really sure what that article is trying to say. Maybe it's just showing examples that technically aren't guaranteed to behave a certain way per the specification of the java memory model, even if in practice they do. There's no concern that this code will ever give the wrong result, but it's fine to proceed with this change.

[cushon]

I think the original code is the same as the buggy double-checked-locking implementation discussed here, except without the locking? This might be OK if it doesn't need to be thread-safe, but the implementation comment doesn't seem very confident of that:

Not expected to be called concurrently, and an idempotent computation if it is.

[coeuvre]

I think Spawn is not supposed to be thread-safe: it is assigned to only one thread for execution. I checked the call sites of getLocalResources and there are not concurrent calls.

[fmeum]

I think Spawn is not supposed to be thread-safe: it is assigned to only one thread for execution. I checked the call sites of getLocalResources and there are not concurrent calls.

Couldn't this end up being called concurrently with dynamic execution and --dynamic_remote_strategy set to a non-remote strategy? It's only a race in theory to begin with and this particular option being set to something other than remote probably also happens only very rarely, but it's not statically obvious to me that this isn't called concurrently.

[coeuvre]

Couldn't this end up being called concurrently with dynamic execution and --dynamic_remote_strategy set to a non-remote strategy?

In practice, only local branch calls this method -- it's for local execution anyway.