Rework lua timers and remove lua.Lock#3023
Merged
dmaluka merged 3 commits intomicro-editor:masterfrom Mar 14, 2024
Merged
Conversation
Directly using Go's time.AfterFunc() from lua is tricky. First, it requires the lua timer callback to explicitly lock ulua.Lock to prevent races. Second, it requires the lua timer callback to explicitly redraw the screen if the callback changes the screen contents (see micro-editor#2923). So instead provide micro's own timer API which ensures both synchronization and redrawing on its own, instead of leaving this burden to lua code. In fact, its implementation runs the lua timer callback in the main micro's goroutine (i.e. from micro's perspective it is synchronous, not asynchronous), so both redrawing and synchronization are ensured automatically. Fixes micro-editor#2923
Since we now expose our own micro.After() API which is more convenient and safer to use than directly using Go timers, we can remove exposing Go timers to lua directly.
Exposing locking primitives to lua plugins is tricky and may lead to deadlocks. Instead, if possible, it's better to ensure all the needed synchonization in micro itself, without leaving this burden to lua code. Since we've added micro.After() timer API and removed exposing Go timers directly to lua, now we (probably?) have no cases of lua code possibly running asynchronously without micro controlling when it is running. So now we can remove lua.Lock. This means breaking compatibility, but, until recently lua.Lock wasn't workable at all (see micro-editor#2945), which suggests that it has never been really used by anyone. So it should be safe to remove it.
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Directly using Go timers from lua (e.g. via Go's
time.AfterFunc()) is tricky. First, it requires the lua timer callback to explicitly locklua.Lockto prevent races. Second, it requires the lua timer callback to explicitly redraw the screen if the callback changes the screen contents (see #2923).So, as per the discussion in #2945, this PR reworks lua timers support: it replaces directly exposed Go's
time.AfterFunc()etc with micro's ownmicro.After()timer API, which ensures both synchronization and redrawing on its own, instead of leaving this burden to lua code.Since this PR removes directly exposing Go timers to Lua, now we (probably?) have no cases of lua code possibly running asynchronously without micro controlling when it is running. So this PR also removes
lua.Lock, since it is not needed anymore. (Before the recent fix #2945,lua.Lockwasn't workable at all, which suggests that it has never been really used by anyone. So it should be safe to remove it.)Fixes #2923