Skip to content

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

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.

Sign up for GitHub

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

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

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 21 additions & 5 deletions pkg/highlight/highlighter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,11 @@ func (h *Highlighter) HighlightStates(input LineStates) {
line := input.LineBytes(i)
// highlights := make(LineMatch)

h.lastRegion = nil
if i > 0 {
h.lastRegion = input.State(i - 1)
}

if i == 0 || h.lastRegion == nil {
h.highlightEmptyRegion(nil, 0, true, i, line, true)
} else {
Expand All @@ -332,6 +337,14 @@ func (h *Highlighter) HighlightStates(input LineStates) {
// It sets all other matches in the buffer to nil to conserve memory
// This assumes that all the states are set correctly
func (h *Highlighter) HighlightMatches(input LineStates, startline, endline int) {
var curState *region
if startline > 0 {
input.Lock()
if startline-1 < input.LinesNum() {
curState = input.State(startline - 1)
}
input.Unlock()
}
for i := startline; i <= endline; i++ {
input.Lock()
if i >= input.LinesNum() {
Expand All @@ -343,11 +356,13 @@ func (h *Highlighter) HighlightMatches(input LineStates, startline, endline int)
highlights := make(LineMatch)

var match LineMatch
if i == 0 || input.State(i-1) == nil {
h.lastRegion = curState
if i == 0 || h.lastRegion == nil {
match = h.highlightEmptyRegion(highlights, 0, true, i, line, false)
} else {
match = h.highlightRegion(highlights, 0, true, i, line, input.State(i-1), false)
match = h.highlightRegion(highlights, 0, true, i, line, h.lastRegion, false)
}
curState = h.lastRegion

input.SetMatch(i, match)
input.Unlock()
Expand All @@ -360,11 +375,11 @@ func (h *Highlighter) HighlightMatches(input LineStates, startline, endline int)
func (h *Highlighter) ReHighlightStates(input LineStates, startline int) int {
// lines := input.LineData()

h.lastRegion = nil
var curState *region
if startline > 0 {
input.Lock()
if startline-1 < input.LinesNum() {
h.lastRegion = input.State(startline - 1)
curState = input.State(startline - 1)
}
input.Unlock()
}
Expand All @@ -379,12 +394,13 @@ func (h *Highlighter) ReHighlightStates(input LineStates, startline int) int {
// highlights := make(LineMatch)

// var match LineMatch
h.lastRegion = curState
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

@dmaluka dmaluka Apr 9, 2024

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

@JoeKar JoeKar Apr 10, 2024

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

@dmaluka dmaluka Apr 10, 2024

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

@JoeKar JoeKar Apr 11, 2024

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.

lastState := input.State(i)

input.SetState(i, curState)
Expand Down