fix: process element usages under ReadAction #714

angelozerr · 2024-12-18T10:16:49Z

fix: process element usages under ReadAction

Fixes #708

angelozerr · 2024-12-18T10:18:54Z

@SCWells72 please review the PR.

As @jansorg have again a problem, I created this PR which process element usages in a ReadAction. The process element usages is not done whene there are a Dumb (to improve performance).

angelozerr · 2024-12-18T10:20:03Z

@jansorg if you want to test the PR now, please read
https://github.com/redhat-developer/lsp4ij?tab=readme-ov-file#testing-the-ci-builds

SCWells72 · 2024-12-18T13:51:16Z

@jansorg, I'm curious about the difference between your IDE environment and ours. I've tried to reproduce these read exceptions on multiple versions of the IDE, multiple IDEs, and multiple OSes. I wouldn't expect the threading model to be different across OSes or IDEs at the same version, but as we all know all too well, it can change across versions. Unfortunately without being able to reproduce these locally at all, it makes it difficult to know when it's properly resolved short of enclosing everything in a read action which has its own issues.

Can you please describe the host OS, IDE product name and version, any additional/non-standard JVM args you're using (i.e., are you running it in dev mode which can be more strict about the threading model?), and what exact steps you're taking when these issues happen?

jansorg · 2024-12-18T13:58:53Z

@SCWells72 I'm on Linux, IntelliJ Ultimate 2024.3.1, using Kotlin in K2 mode (happens without K2 too, I think)

The only possible cause I can think of is -Didea.is.internal=true, which may enable stricter checking, but I'm not sure.

The exception happens when I invoke a normal "find usages" popup via keyboard. It's really nothing out of the ordinary.

IntelliJ IDEA 2024.3.1 (Ultimate Edition)
Build #IU-243.22562.145, built on December 9, 2024
Runtime version: 21.0.5+8-b631.28 amd64 (JCEF 122.1.9)
VM: OpenJDK 64-Bit Server VM by JetBrains s.r.o.
Toolkit: sun.awt.X11.XToolkit
Linux 6.12.4-arch1-1
Kotlin plugin: K2 mode
Kotlin analyzer version: 2.1.20-ij243-27
GC: G1 Young Generation, G1 Concurrent GC, G1 Old Generation
Memory: 8192M
Cores: 32
Registry:
  batch.inspections.modal.progress.when.building.global.reference.graph=false
  ide.allow.merge.buttons=false
  ide.experimental.ui=true
  ide.ui.new.file.chooser=true
  i18n.locale=
Non-Bundled Plugins:
  com.redhat.devtools.lsp4ij (0.9.1-20241217-193530)
  DevKit (243.23535)
  YourKit Java Profiler 2024.3-b152 Integration (2024.3.152.0)
  org.jetbrains.idea.grammar (2022.3.2)
  com.intellij.uiDesigner (243.22562.145)
  com.intellij.properties.bundle.editor (243.21565.122)
  org.jetbrains.plugins.go (243.22562.145)
  ZKM (243.21565.129)
  com.cppcxy.Intellij-SumnekoLua (3.13.0.43-IDEA243)
  pro.bashsupport (4.5.0.243)
  org.jusecase.jte-intellij (2.2.1)
  com.github.copilot (1.5.29.7524)
  com.intellij.wiremock (243.22562.13)
Kotlin: 243.22562.145-IJ
Current Desktop: KDE

SCWells72 · 2024-12-18T14:05:24Z

The only possible cause I can think of is -Didea.is.internal=true, which may enable stricter checking, but I'm not sure.

Okay. All of my dev IDE instances are also run with that flag, but I'll confirm that again as well.

The exception happens when I invoke a normal "find usages" popup via keyboard. It's really nothing out of the ordinary.

To be clear as there are multiple "find usages" actions and I've seen behavioral differences between them in the past, is this happening with the one that shows the results in a tool window (Find Usages / Alt+Shift+7), the one that shows results in a pop-up menu (Show Usages / Ctrl+Alt+7), or both?

angelozerr · 2024-12-18T14:53:19Z

@SCWells72 you have more skills than me about IJ, but the thing that I have seen in IJ community base code is that it opens one read action and do some work instead of opening several on some each code part.

It is very hard to know xhich code call can throw ReadAction exception.

I understand that your last fix was to open ReadAction for some line code to avoid some block, but this code is very hard to maintain.

I think opening one readaction should be enough.

SCWells72 · 2024-12-18T15:08:24Z

I think opening one readaction should be enough.

@angelozerr, yeah, I'm fine with that. My concern is that @jansorg is seeing issues that we're not. The most recent one he's seeing has nothing to do with the external references support that I added. It's occurring in the first line of that method when getting the element's containing file. That should happen consistently, or it shouldn't happen consistently. I don't like that he's seeing it and we're not. That inconsistency makes finding these types of threading issues during dev/test very difficult, and we definitely don't want to go the route of adding coarse-grained read (or worse, write) actions as de facto to defend against such inconsistency. That's why I'm trying to understand and isolate these differences so that I can figure out how we can see (and address) the corresponding issues during dev/test.

jansorg · 2024-12-18T15:51:00Z

@SCWells72 I'm invoking Ctrl+B (Navigate > Declaration or Usages)

SCWells72 · 2024-12-18T16:13:24Z

@SCWells72 I'm invoking Ctrl+B (Navigate > Declaration or Usages)

Ah, okay. You're doing that on the declaration itself, correct?

jansorg · 2024-12-18T16:15:39Z

I usually invoke use this action anywhere.
I just tried this on the declaration and this caused the exception (this time in Java code).

SCWells72 · 2024-12-18T16:24:19Z

Okay. Interestingly Ctrl/Cmd+B doesn't even trigger LSPUsageSearcher.processElementUsages() from an LSP file -- specifically one that's fundamentally plain text or TextMate underneath -- presumably because there's no real PSI tree for those files. I was able to hit a breakpoint on the first line of that method by using that keyboard shortcut from a declaration within a file with a true file type, e.g., Java. I did not, however, reproduce the read access error when I did so, but @angelozerr mentioned that he'd seen it only once himself, and the exact same operation didn't reproduce it for him, so there must be something around PSI caching or similar involved as well.

So obviously adding a read action around the entire implementation of processElementUsages() "fixes" this instance, but it sounds like additional caution is required around the initial checks performed by LSP4IJ EPs that can be invoked in the context of any file type/language and not just TEXT/textmate.

SCWells72 · 2024-12-18T16:26:07Z

src/main/java/com/redhat/devtools/lsp4ij/usages/LSPUsageSearcher.java

-    private static Position getPosition(@NotNull PsiElement element, @NotNull PsiFile psiFile) {
-        if (ApplicationManager.getApplication().isReadAccessAllowed()) {
-            return doGetPosition(element, psiFile);
+    private static void processElementUsagesUnderReadAction(@NotNull ThrowableComputable<CodeVisionState, Throwable> computable,


Everything in this change makes sense to me except for the addition of this method. Why was this added instead of just wrapping the entire body of processElementUsages() in ReadAction.compute()?

The main reason is because when dumb is processed we need to skip this search. And when we are in non EDT it should be cancellable.

I find this logic code in UJ ultimate repo.

Fixes redhat-developer#708

angelozerr · 2024-12-18T18:50:36Z

Thanks @SCWells72 @jansorg for your reviews.

angelozerr · 2024-12-18T18:52:23Z

Again @jansorg once https://github.com/redhat-developer/lsp4ij/actions/runs/12399426259 will be finished please install nighty and give us feedback.

jansorg · 2024-12-18T21:25:05Z

@angelozerr With the latest build, I did not see this exception again. Thanks!

SCWells72 reviewed Dec 18, 2024

View reviewed changes

angelozerr force-pushed the find_usages_readaction branch from 174442e to d485d53 Compare December 18, 2024 18:34

fix: process element usages under ReadAction

54caf44

Fixes redhat-developer#708

angelozerr force-pushed the find_usages_readaction branch from d485d53 to 54caf44 Compare December 18, 2024 18:46

SCWells72 approved these changes Dec 18, 2024

View reviewed changes

angelozerr merged commit 246439d into redhat-developer:main Dec 18, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: process element usages under ReadAction #714

fix: process element usages under ReadAction #714

angelozerr commented Dec 18, 2024

angelozerr commented Dec 18, 2024

angelozerr commented Dec 18, 2024 •

edited

Loading

SCWells72 commented Dec 18, 2024 •

edited

Loading

jansorg commented Dec 18, 2024

SCWells72 commented Dec 18, 2024

angelozerr commented Dec 18, 2024

SCWells72 commented Dec 18, 2024

jansorg commented Dec 18, 2024

SCWells72 commented Dec 18, 2024

jansorg commented Dec 18, 2024

SCWells72 commented Dec 18, 2024

SCWells72 Dec 18, 2024

angelozerr Dec 18, 2024

angelozerr commented Dec 18, 2024

angelozerr commented Dec 18, 2024

jansorg commented Dec 18, 2024

fix: process element usages under ReadAction #714

fix: process element usages under ReadAction #714

Conversation

angelozerr commented Dec 18, 2024

angelozerr commented Dec 18, 2024

angelozerr commented Dec 18, 2024 • edited Loading

SCWells72 commented Dec 18, 2024 • edited Loading

jansorg commented Dec 18, 2024

SCWells72 commented Dec 18, 2024

angelozerr commented Dec 18, 2024

SCWells72 commented Dec 18, 2024

jansorg commented Dec 18, 2024

SCWells72 commented Dec 18, 2024

jansorg commented Dec 18, 2024

SCWells72 commented Dec 18, 2024

SCWells72 Dec 18, 2024

Choose a reason for hiding this comment

angelozerr Dec 18, 2024

Choose a reason for hiding this comment

angelozerr commented Dec 18, 2024

angelozerr commented Dec 18, 2024

jansorg commented Dec 18, 2024

angelozerr commented Dec 18, 2024 •

edited

Loading

SCWells72 commented Dec 18, 2024 •

edited

Loading