Lazy init in PSI classes and multi-threaded access

Thanks, Peter!

Updating just the references in a synchronized block is a good idea, I didn't think of that.

I ended up to mark all fields which are part of the internal state of a PSI element as volatile. If there's more than one field it has to be updated in a synchronized block to avoid a corrupted state (multiple volatile fields updated concurrently might end up in an invalid combination in a worst-case scenario). I'm using volatile to avoid the synchronize on read access.

I didn't spot the volatile flag in the first sample, sorry.

For later readers, this is what I'm using now:

private final Object stateLock = new Object();
private volatile X x; //volatile to avoid synchronize on read-access

public X getX() {
  if (x == null) {
    X newX = ... //compute new value;

    synchronized (stateLock) {
      //you could check for x==null here if you prefer
      x = newX;
    }
  }

  return x;
}

public void subtreeChanged() {
  synchronized (stateLock) {
    x = null;
    //reset all fields which are part of the internal state
  }
}

Unless you really check for x==null inside the first "synchronized", I'd just drop it as it seems to add no value.

BTW if you modify several fields together (in a "synchronized"), then reading them should probably be also done in a "synchronized", otherwise other threads might still observe partially updated state.

Thanks for the reply! The first synchronize could be removed here, right.

I thought a bit more about this and wrote it down at https://www.plugin-dev.com/intellij/custom-language/psi-element-caching/ .

I'm not totally sure about partially updated state. I (currently) think that there shouldn't be a partially updated state under the assumption that reading data doesn't overlap with a call to subtreeChanged(). Parallel reads in different threads are all based on the same PSI (with no parallel write action). (If subtreeChanged() was called at any time then everything in a method had to be in a single synchronized block, I think.)

After some analysis my assumptions are that

- subtreeChanged() is always called in a write action

- methods which update the PSI element's cache are wrapped in a read or write action, i.e. parallel reads or one writer at a time

- accessing a PsiElement's or ASTNode's properties doesn't involve further locking

Further questions if you have the time for that:

- Would you recommend to always do access to PSI / ASTNode properties outside of a synchronized block or is it usually safe to do so if no stub index, etc. is used?

- Is it safe in general to access a StubIndex in a PsiElement (this, of course, outside of a synchronized block)? I couldn't find out.

Thanks a lot!

Your assumptions are correct for physical PSI. There can also be non-physical PSI not corresponding to any file in project (e.g. created on the fly from text). It can be modified in any thread, and clients take full responsibility for synchronizing it themselves.

Yes, I'd recommend to call any nontrivial code outside of "synchronized", unless you really want that computation to be run on not more than one thread. In that case, you should be very careful about potential deadlock. But luckily, this need seems to be extremely rare.

Accessing StubIndex from PsiElement should be safe (unless you do it during a write operation on that PSI, which could lead to inconsistencies). But most of the PSI doesn't do that: it's mostly concerned with local stuff, i.e. the surrounding AST nodes. Index is usually needed for a higher-level functionality, like completion, goto popups etc. But also for reference.resolve(), which you might consider part of PSI (or might not).