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

fix: cache indent matches to avoid occasional race conditions #640

Conversation

PriceHiller
Copy link
Contributor

@PriceHiller PriceHiller commented Dec 28, 2023

This is a really weird bug. I know it's an issue with how the get_matches function works and buftick. indentexpr isn't really intended for indenting many lines that all depend on each other's indents.

What this does is "freeze" the matches during a indent operation against a range of lines and only updates the previous matches' indent as necessary. By doing this we get "memory" for the indentexpr of the previous indents as part of a ranged indent operation like norm! 0gg=G which avoids any race conditions as to the speed of indents. Without this caching the get_matches function is called on every indentexpr operation, so for every line in the range, which can result in bad data back for calculating the indents.

I have added a big ol' comment above the indentexpr with further information.

Closes #639

@PriceHiller
Copy link
Contributor Author

Proof this works for me:

orgmode-indent-race-condition-fixed

I only let this run out to near 30 times on Neovim 0.9.0. Max upload size is 10mb for files on github 🤷. I had another that went out to around ~500 runs before I figured that was good enough:
image

The script I ran to validate:

TEST_RUN=1; while :; do
  printf "==== Current Test Run: %s ====\n" "${TEST_RUN}"
  if make test FILE=./tests/plenary/org/indent_spec.lua; then
    printf "\n\n++++ Finished Test Run: %s ++++\n\n" "${TEST_RUN}"
  else
    printf "\n\n==== Tests Failed on Run %s ====\n\n" "${TEST_RUN}"
    break
  fi
  ((TEST_RUN++))
done

I recommend running the above on my branch and validating it works for you as well, the script will stop if any failures occur. AFAIK my patch resolves the issue.

@PriceHiller PriceHiller mentioned this pull request Dec 28, 2023
Copy link
Member

@kristijanhusak kristijanhusak left a comment

Choose a reason for hiding this comment

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

Could we slightly simplify this by leveraging options on the memoize_by_buf_tick (https://github.com/nvim-treesitter/nvim-treesitter/blob/master/doc/nvim-treesitter.txt#L502) ? You can provide a custom key where the memoization knows to use either cached result or generate a new one. Same thing is done in markup highlighter here

key = function(bufnr, line_index)

@PriceHiller
Copy link
Contributor Author

So I'm not certain that key function actually works for this use case. I'd love to be proven wrong, but as far as I can tell the key function doesn't lend itself well to this problem.

It seems the key function has a hard requirement on using the vim.b.changedtick to determine if the cache should be invalidated, seen here. Unfortunately it's possible the buffer's tick gets changed during indent operations (which I suspect is what was causing the cache invalidation). There doesn't seem to be a way to avoid this using the memoize_by_buf_tick function. We really only want to cache entirely by previous line numbers and the vim mode.

If you know of a way to do this with the key function, I'm happy to implement it. But I don't see a decent way of doing it that isn't just shifting the current logic around the key function and doing our level best to ignore the changedtick - kinda hacky.

Copy link
Member

@kristijanhusak kristijanhusak left a comment

Choose a reason for hiding this comment

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

Yeah you are right. Lets go with this for now. Thanks!

@kristijanhusak kristijanhusak merged commit 433c70a into nvim-orgmode:master Jan 4, 2024
6 checks passed
@PriceHiller PriceHiller deleted the fix/indentexpr-race-condition branch February 15, 2024 04:07
SlayerOfTheBad pushed a commit to SlayerOfTheBad/orgmode that referenced this pull request Aug 16, 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.

Tests fail for full reindents with indent mode on non-nightly (0.10.0) Neovim
2 participants