Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

highlighter: Fix last open racy access in the *Highlight*() functions #3242

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JoeKar
Copy link
Collaborator

@JoeKar JoeKar commented Apr 9, 2024

Otherwise it's possible that *Highlight*() functions called from the async initial highlighting can be interrupted by the main goroutine, which then changes the already set and furthermore used h.lastRegion. The resulting last region respective set state isn't reliable any longer then.

The need for this came up in #3237.

@JoeKar JoeKar requested a review from dmaluka April 9, 2024 20:41
if i == 0 || h.lastRegion == nil {
h.highlightEmptyRegion(nil, 0, true, i, line, true)
} else {
h.highlightRegion(nil, 0, true, i, line, h.lastRegion, true)
}
curState := h.lastRegion
curState = h.lastRegion
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But h.lastRegion is modified also by highlightRegion() and highlightEmptyRegion(), which are also called from both goroutines, so we still have the race, right?

I was actually thinking about removing this lastRegion field from the Highlighter struct, making it just a local variable, passing it to highlightRegion() and highlightEmptyRegion() (like we already do), and modifying highlightRegion() and highlightEmptyRegion() so that they don't set this global h.lastRegion but just return the updated value of lastRegion to the caller.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But h.lastRegion is modified also by highlightRegion() and highlightEmptyRegion(), which are also called from both goroutines, so we still have the race, right?

h.lastRegion is touched by the re-highlighting too, but only in case it can obtain the lock, which is in the moment of async access to h.lastRegion held by the initial highlighting. So the lines should be processed consistent each after an other.
In case h.lastRegion is problematic, then this would be valid for every other member inside the Highlighter struct too, which is set inside the highlighting process. Maybe you saw it already...I extend this structure in my rework.

I was actually thinking about removing this lastRegion field from the Highlighter struct [...]

I know. Regarding the usage of h.lastRegion outside of h.highlight*() I'm on your side. It looks ugly and should be prevent, but I wasn't able to remove it without breaking anything. I will try to get it working in the rework first and then we can see if it's feasible for the old approach too, when the minimal invasive change here isn't enough.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the lock is held only while processing a single line.

  1. initial highlighting locks the lock, processes line 2000, updates h.lastRegion and unlocks the lock
  2. rehighlighting locks the lock, processes line 50, updates h.lastRegion and unlocks the lock
  3. initial highlighting locks the lock and processes line 2001 using h.lastRegion value set by rehighlighting for line 50, not for line 2000 .....

Or am I missing something that ensures that this will not happen?

In case h.lastRegion is problematic, then this would be valid for every other member inside the Highlighter struct too, which is set inside the highlighting process. Maybe you saw it already...I extend this structure in my rework.

Hmm, no I didn't. (I still had no time to get really familiar with your #3127, nor with the original code inside highlightRegion() and highlightEmptyRegion(), for that matter.) Now I see you extended it. Ok, yes, if h.lastRegion is problematic, those extended fields are problematic too, for the same reason. Well, why not just pack those extended fields into a "local state" structure, and pass this structure (or a pointer to it) between highlighter functions, within a single goroutine, instead of it being a global state shared between goroutines, - just like in the case of lastRegion alone in the approach I've been suggesting?

Although I realize that this approach (passing of local lastRegion or "local state" structure between functions) might be not as easy as it seems, since highlightRegion() and highlightEmptyRegion() are recursive, so need to figure out what exactly to pass between those recursive calls...

I will try to get it working in the rework first and then we can see if it's feasible for the old approach too, when the minimal invasive change here isn't enough.

Sure, it's up to you what you do first. Maybe it's indeed better to focus on #3127 first, after all this race we are trying to tackle here is purely theoretical so far, we don't even know how to reproduce it, while #3127 fixes some actual observed issues.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I didn't oversee something, then h.lastRegion should be set before the usage again with the previous state.
The better style will be applied in #3127.

Otherwise it's possible that `*Highlight*()` functions called from the async
initial highlighting can be interrupted by the main goroutine, which then
changes the already set and furthermore used `h.lastRegion`. The resulting
last region respective set state isn't reliable any longer then.
@JoeKar JoeKar changed the title highlighter: Fix last open racy access in ReHighlightStates() highlighter: Fix last open racy access in the *Highlight*() functions Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants