zed icon indicating copy to clipboard operation
zed copied to clipboard

Fix race condition between AutoindentMode and "Formatting on typing"

Open ferzisdis opened this issue 9 months ago • 5 comments

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:

  1. 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.
  2. 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

ferzisdis avatar May 24 '25 15:05 ferzisdis

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:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot[bot] avatar May 24 '25 15:05 cla-bot[bot]

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

probably-neb avatar May 27 '25 14:05 probably-neb

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:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot[bot] avatar May 27 '25 23:05 cla-bot[bot]

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.

ferzisdis avatar May 27 '25 23:05 ferzisdis

Is this ready for review? If so, could you please resolve the cla-bot issue?

probably-neb avatar Jun 03 '25 07:06 probably-neb

@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.

ferzisdis avatar Jun 03 '25 18:06 ferzisdis