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

[JoeKar]

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.

[dmaluka]

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.

[JoeKar]

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.

[dmaluka]

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.

[JoeKar]

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.