Fix race condition between AutoindentMode and "Formatting on typing"
This PR addresses to fix (#31308) a race condition where AutoindentMode (in buffer.cs) and "Formatting on typing" (in lap_store.rs) concurrently calculate indentation using the same buffer snapshot.
I considered several approaches to fix the issue, but each has its drawbacks:
- Disabling AutoindentMode for Rust in settings – This is one of the simplest and correct solutions, but it doesn’t address the race condition systematically, since the same problem could occur for other languages.
- Detecting buffer changes during indentation calculation – If the buffer is modified mid-calculation, recompute the indentation. Pros: A robust solution that ensures correctness. Cons: Increased resource usage due to repeated computations; Risk of infinite loops if the buffer is updated continuously.
Final Solution: I implemented a straightforward method to detect and discard duplicate parallel changes.
How it works:
- The system checks if identical modifications were made concurrently.
- If a change is flagged as a duplicate, it gets discarded.
Trade-offs:
- While this approach may not be universally optimal for all edge cases,
- It effectively resolves this specific race condition without overengineering.
I'm happy to make any necessary adjustments to ensure this code aligns better with your codebase and project goals
Issue:
If AutoindentMode finishes first, formatting works correctly. If "Formatting on typing" starts before AutoindentMode completes, it results in double indentation.
Closes #31308
Release Notes:
- N/A
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: ferzisdis. This is most likely caused by a git client misconfiguration; please make sure to:
- check if your git client is configured with an email to sign commits
git config --list | grep email - If not, set it up using
git config --global user.email [email protected] - Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails
Hey, I think this is an interesting solution for the problem, but I'm wondering if there's a more specific and simple solution we could do. Did you consider making the auto-indent and format requests happen serially? They seem fundamentally at odds.
Also, I'm concerned that merging is not what we should be doing to resolve conflicts between auto-indent and auto-format. To me, auto-indent is a nicety that makes your cursor jump around less, while formatting is more along the lines of "this is how I want the code to look before I commit / make a PR", so I feel like what we want is not the merged result, we want the result of the auto-formatting, i.e. we should do auto-indent before formatting so the format result is the final result
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: ferzisdis. This is most likely caused by a git client misconfiguration; please make sure to:
- check if your git client is configured with an email to sign commits
git config --list | grep email - If not, set it up using
git config --global user.email [email protected] - Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails
Hey @probably-neb, Thanks for the review and great suggestions!
I did consider making the requests sequential initially, but dismissed the idea because I was concerned about potential delays in delivering the final formatted result to users (compared to the merge solution).
However, after your comment, I reconsidered and realized that for my use case (which I demonstrated in the bug), on-type-formatting should always produce identical results to auto-indent. This means on-type-formatting would typically produce empty edit lists anyway, so waiting a bit for auto-indent to finish shouldn't cause any issues.
I've updated the PR to implement the sequential solution we discussed.
Is this ready for review? If so, could you please resolve the cla-bot issue?
@probably-neb , sorry for missing the cla-bot issue. I resolved it by recreating the pull request with a fresh commit history: https://github.com/zed-industries/zed/pull/32005
This PR has to be closed.