Threading issues about IndexUtilBase and PSIElement

[Ezrnest]

IndexUtilBase provides functionalities to get certain PSIElements in the file set.
However, I am confused with some threading issues here.

Regarding the PSIElements

From the Intellij SDK, PSIElement is mutable and not thread safe, so we should obtain the read access when dealing with them.
However, as PSIElements are collected and returned, we supposed to also acquire the read access if we want to further process them, leading to some more trouble. For example, EnvironmentManager.findAllAliases forgets to obtain the read access, while CommandManager.findAllAliases uses the thread-blocking read-access method.

Perhaps we should create separate models that record the essential information that we want during the indexing process, which are immutable and thread-safe. Additionally, they can reduce the difficulty of extracting information from the PSI tree during downstream processing.

Broken read accesses?

I sometimes encounter codes like

indexedDefinitions.filter {
    // Assume the parameter definition has the command being defined in the first required parameter,
    // and the command definition itself in the second
    runReadAction { it.requiredParameter(1)?.containsAny(aliasSet) == true }
}
    .mapNotNull { runReadAction { it.requiredParameter(0) } }
    .forEach { registerAlias(firstAlias, it) }

where we start lots of runReadAction separately. It seems to me that even for the same command, it.requiredParameter(1) can possibly change after after the filtering, so the final results can be actually broken?
I am not sure about it, but it seems that we should wrap all the code in the readAction block and avoid leaking the PSIElements outside the block.

[PHPirates]

Perhaps we should create separate models that record the essential information that we want during the indexing process, which are immutable and thread-safe.

But if we work on processed information, then it will be out of date more often? At least right now, if you take e.g. psiElement.text then it will be up to date, though slow to execute.
For things that we cache already then yes, it would be better to save the properties we need instead of the psi element itself, but that's quite some work I think and not sure what the benefit would be.

It seems to me that even for the same command, it.requiredParameter(1) can possibly change after after the filtering, so the final results can be actually broken?

Yes, this is explicitly mentioned in the SDK documentation:

The read objects aren't guaranteed to survive between several consecutive read actions. Whenever starting a read action, check if the PSI/VFS/project/module is still valid.

However, if we put the whole block in a read action, it may be too slow and block the UI (for the classic read action, with the coroutine write allowing read action I think this is not so much of a problem). Theoretically we would need to check psi element validity at the start of every read action, which is not very convenient, but I don't see a good solution.

[Ezrnest]

After browsing the source code and running stack trace, I find that in most cases the read access is already granted outside the extension's entry point, such as the checkFile method in inspections. See the source of InspectionRunner.executeInImpatientReadAction. Therefore, there is no need to get the read access by our own.

If we use the StubIndex properly (by locating exactly the relavent elements, which are supposed to be limited in number), then it would not cause much performance problem and we do not need to create caches manually. Moreover, the inspections and other many tasks are run in background, so they will not block the UI.

However, since the user can customize all the commands in latex, sometimes we have to traverse all the commands in all the files and the operation would be very slow. In this case, we have to cache the results manually and schdule background updates (where we have to get read access).
In this case, I think that we should save only the final result to speed up the computation.

But if we work on processed information, then it will be out of date more often? At least right now, if you take e.g. psiElement.text then it will be up to date, though slow to execute.

After leaving the read action block, the PsiElement can be modified or even removed from the tree, so PsiElement.text can also be invalid? So perhaps keeping those PsiElements does not help to be up to date, and we'd rather cache the processed information.

I believe that by refactoring these parts and optimizing the implementations, we can speed up the indexing process and also avoid potential thread-safety problems.
I think the following changes would be benefitial:

• Remove the unnecessary read action block for direct access without cache.
• Implement a new caching mechanism that caches only the final information.
  I can work on a prototype of it.

[PHPirates]

Yes, in most cases we do not need to acquire read access, but because I was not aware of the performance impact (not sure why it doesn't only do something when necessary, by default) you have seen that I littered read actions everywhere they weren't necessarily needed. Not sure if this can be checked generally

Ah, if the psi element is modified, then psiElement.text will become invalid? I thought it would return the new text, but I never checked I think. If not then we can just as well store the text indeed.
I may have been confused with stubs, where for example LatexCommands.name will return the stub value, which may be out of date, but LatexCommands.commandToken.text will use the AST and will be up to date (but slower).