fix: Addressed issues with LSP Find Usages implementation (issue 708) #711
Conversation
…ead action. I'm unfortunately unable to reproduce the actual error seen by the reporting user in 2023.2 or 2024.3, but wrapping the entire call in a read action should resolve all downstream requirements for the read lock.
…cope of the read action, ensures that the search scope from the Find Usages dialog is used, and ensures that the LSP4IJ find usages handler is only used if there is at least one configured language server definition for the file that supports the usages capability.
| */ | ||
| public static void processExternalReferences(@NotNull PsiFile file, | ||
| int offset, | ||
| @NotNull SearchScope searchScope, |
There was a problem hiding this comment.
For Find Usages, we need the configured search scope.
| */ | ||
| public static void processExternalReferences(@NotNull PsiFile file, | ||
| int offset, | ||
| @NotNull Processor<PsiReference> processor) { | ||
| processExternalReferences(file, offset, ReadAction.compute(file::getUseScope), processor); |
There was a problem hiding this comment.
This is the original signature that derives the search scope from the file.
| VirtualFile virtualFile = file.getVirtualFile(); | ||
| if (virtualFile != null) { | ||
| Document document = LSPIJUtils.getDocument(virtualFile); | ||
| TextRange wordTextRange = document != null ? LSPIJUtils.getWordRangeAt(document, file, offset) : null; | ||
| if (wordTextRange != null) { | ||
| LSPPsiElement wordElement = new LSPPsiElement(file, wordTextRange); | ||
| String wordText = wordElement.getText(); | ||
| String wordText = ReadAction.compute(wordElement::getText); |
There was a problem hiding this comment.
Tactical read actions where required.
| @@ -95,7 +113,7 @@ private static void processExternalReferences(@NotNull PsiFile file, | |||
| } | |||
|
|
|||
| Set<String> externalReferenceKeys = new HashSet<>(); | |||
| PsiSearchHelper.getInstance(project).processElementsWithWord( | |||
| ReadAction.run(() -> PsiSearchHelper.getInstance(project).processElementsWithWord( | |||
There was a problem hiding this comment.
Broader read action around the word search proper.
| return; | ||
| } | ||
|
|
||
| Project project = element.getProject(); |
There was a problem hiding this comment.
Don't proceed if there are no configured language server definitions for this file that support usages. That really addresses Joachim's core issue where it was doing this work in a file that shouldn't have been considered by LSP4IJ at all.
| LSPExternalReferencesFinder.processExternalReferences(file, element.getTextOffset(), reference -> processor.process(new UsageInfo2UsageAdapter(new UsageInfo(reference)))); | ||
| LSPExternalReferencesFinder.processExternalReferences( | ||
| file, | ||
| ReadAction.compute(element::getTextOffset), |
There was a problem hiding this comment.
This is the specific call that was causing the exception in Joachim's report. Now we get the text offset in a read action.
SCWells72
changed the title
| file, | ||
| ReadAction.compute(element::getTextOffset), | ||
| options.searchScope, | ||
| reference -> processor.process(new UsageInfo2UsageAdapter(new UsageInfo(reference))) |
There was a problem hiding this comment.
If I remember you can have a ReadAction problem here. (See the first call process.process which compute a ReadAction
There was a problem hiding this comment.
Do you mean that new UsageInfo2UsageAdapter(new UsageInfo(reference)) should be enclosed in a ReadAction here like in the other similar constructor calls in the same method. I don't think that's necessary because the UsageInfo2UsageAdapter constructor already includes its own ReadAction, and the UsageInfo constructor also looks fine. Having said that, I'm totally fine wrapping that in a ReadAction both for consistency and out of a sense of caution if you'd like. Let me know.
There was a problem hiding this comment.
I went ahead and added it.
There was a problem hiding this comment.
Yes it is that, I don't remember how to reproduce it, but I had to add ReadAction#compute to fix the problem. See
Fixes redhat-developer#702 Signed-off-by: azerr <[email protected]>
|
@SCWells72 please fix conflicts. |
|
@SCWells72 instead of having ReadAction run/compute in several locations, perhaps it should better to have just one ReadAction, no? |
|
That's how I originally had it. However, read and write actions do take out locks, and it's best to limit the scope of those locks if possible. That's why I've added these in a more fine-grained manner. |
…ead action. I'm unfortunately unable to reproduce the actual error seen by the reporting user in 2023.2 or 2024.3, but wrapping the entire call in a read action should resolve all downstream requirements for the read lock.
…cope of the read action, ensures that the search scope from the Find Usages dialog is used, and ensures that the LSP4IJ find usages handler is only used if there is at least one configured language server definition for the file that supports the usages capability.
# Conflicts: # src/main/java/com/redhat/devtools/lsp4ij/usages/LSPUsageSearcher.java
|
Going to open a new PR for this. |
I'm unfortunately unable to reproduce the actual error seen by the reporting user in both 2023.2 or 2024.3, but wrapping the entire call in a read action should resolve all downstream requirements for the read lock.