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

Avoid file locks by being smarter about when we try to rebuild our information #3737

Merged
merged 12 commits into from
May 21, 2024

Conversation

anaisbetts
Copy link
Contributor

@anaisbetts anaisbetts commented May 8, 2024

On Windows, file locks are enforced, meaning that if we try to access files right after they have been written to, it is highly likely that we'll run into locking issues - either by our own app, Antivirus, or by the application that caused the change (many IDEs and editors are also doing similar operations because they also support Git)

Furthermore even on platforms that we don't have to worry about locking, being aggressive about responding to file change notifications means that we will end up looking at a bunch of files that are changing in-progress, especially with projects that do large changes all at once (i.e. Gradle sync in Android projects)

The Plan - scan when we need it, and when the notifications are quiet

So instead of maintaining a very low debounce threshold of 100ms, instead we are going to make the debounce threshold much, much higher - this timeout will only fire when the project folder is effectively constantly changing. In addition however, we're going to manually flush notifications in two scenarios:

  • If the files in the project are no longer changing - if change notifications have not come in for longer than 1500ms, we will flush any pending change notifications. This gets us out of the way of other tools while still keeping us relatively up-to-date

  • When the window gets focus, flush immediately - this is where we definitely need the most up-to-date information, because the user is about to do something

TODO:

  • Write a version of notify-debouncer-full that allows us to hint when we should flush changes
  • Plumb this through gitbutler-watcher
  • Flush on window focus
  • Flush on system idle

Discussion at https://discord.com/channels/1060193121130000425/1204524097241878629/1237028051800297543

@anaisbetts anaisbetts force-pushed the flushable-debounce branch 3 times, most recently from b4bbeb2 to ae2bcc5 Compare May 13, 2024 11:11
@anaisbetts anaisbetts marked this pull request as ready for review May 15, 2024 15:13
@anaisbetts anaisbetts force-pushed the flushable-debounce branch 3 times, most recently from 589eea3 to e91b9cf Compare May 15, 2024 16:14
We need to make some changes to notify_debounce_full and it seems like
the project is unmaintained, let's copy-paste some code
This adds a new method to allow us to explicitly flush pending change
notifications
Optionally, if we detect that no change notifications have appeared for
a certain number of ticks, we will automatically flush pending
notifications
Give the main window a mechanism to flush pending changes via a Channel
Change the strategy of how timeouts work, from a simple timer, to
something a bit more nuanced
@krlvi
Copy link
Member

krlvi commented May 20, 2024

Hey! A had a few questions:

if change notifications have not come in for longer than 1500ms

Does this mean that in most cases (while no build etc is running) this will be the time until the app can update it's state after a file change? Or is this threshold applied only to subsequent events?
If it's applied to all events, I wonder if a lower value (e.g. 700ms) could also achieve avoiding of the locking, while having a more 'responsive' feel.

Thinking of the scenario where the app is on a separate monitor but not in focus

@anaisbetts
Copy link
Contributor Author

If it's applied to all events, I wonder if a lower value (e.g. 700ms) could also achieve avoiding of the locking, while having a more 'responsive' feel.

We can tweak this number of course, but I wouldn't worry as much about the multiple-monitor scenario - they still won't really need this information until they go to commit, in which case they'll give the window focus and it'll update

* upstream/master: (85 commits)
  History and minor UI fixes (gitbutlerapp#3797)
  History fixes (gitbutlerapp#3796)
  make GitGuardian happy
  bump playwright version
  add vitest exclude for node_modules
  Remove unused exports from ipc
  Update auth.ts to use absolute import
  Use correct invoke function
  setup for async tasks on oplog update
  formatting
  save and restore the gitbutler/integration branch
  remove unneccessary CSPs
  Fix ollama request security
  fix: forgot the port
  feat: Set default snapshot lines threshold to 20 when undefined
  fix: Updated connect-src to allow local connections for ollama
  Minor tweaks to CSS and use real branch name
  Unmount history component when hidden
  Fix bug leaving PR button visible
  Small refactor of history component
  ...
@Caleb-T-Owens
Copy link
Contributor

@krlvi The version control tab in VSCode often takes a second to update - and that is on the same screen! Especially with the focus always performing a flush, if it performs better with the 1.5s debounce, I don't think we ought to worry too much.

@krlvi krlvi merged commit 9f31862 into gitbutlerapp:master May 21, 2024
48 checks passed
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.

None yet

3 participants