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

Async indexing #371

Merged
merged 3 commits into from
Sep 19, 2023
Merged

Async indexing #371

merged 3 commits into from
Sep 19, 2023

Conversation

zachallaun
Copy link
Collaborator

Depending on the computer and size of the project being indexed, it was possible for indexing to take longer than the previous 20 second timeout. Additionally, since indexing was blocking the store GenServer, any calls made to the Store during a long indexing would be likely to time out (default GenServer call timeout is 5 seconds).

This PR updates the Store.State API to load asynchronously so that the store can continue to respond while indexing occurs in a task, while the indexing itself has had its timeout changed to :infinity.

@zachallaun zachallaun requested a review from scohen September 16, 2023 18:37
@zachallaun zachallaun force-pushed the za-async-indexing branch 2 times, most recently from ec30636 to bcc6185 Compare September 16, 2023 21:31
@philipgiuliani
Copy link
Contributor

I've tested it in a project which I couldn't use with Lexical because it was in an indexing loop. Using this branch it worked now and the LSP seems to be fully functional.

@zachallaun zachallaun mentioned this pull request Sep 19, 2023
Copy link
Collaborator

@scohen scohen left a comment

Choose a reason for hiding this comment

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

Note: if you want to change a name, I'm fine with that, but that should be done in a separate PR. A lot of this one is just renaming refresh to update, which makes it a lot harder to review.

@zachallaun
Copy link
Collaborator Author

Note: if you want to change a name, I'm fine with that, but that should be done in a separate PR. A lot of this one is just renaming refresh to update, which makes it a lot harder to review.

Yep, good point. I tend to best understand code by refactoring it, because it helps me understand what the code is doing and helps develop my mental model. In this particular case, I was initially thrown off by the use of refresh and update somewhat interchangeably, so I decided to normalize it to update. I agree that it should have been a separate PR, though.

Depending on the computer and size of the project being indexed, it was
possible for indexing to take longer than the previous 20 second
timeout. Additionally, since indexing was blocking the store GenServer,
any calls made to the Store during a long indexing would be likely to
time out (default GenServer call timeout is 5 seconds).

This commit updates the `Store.State` API to load asynchronously so that
the store can continue to respond while indexing occurs in a task, while
the indexing itself has had its timeout changed to `:infinity`.
@zachallaun zachallaun requested a review from scohen September 19, 2023 18:27
@scohen scohen merged commit eeef7ea into main Sep 19, 2023
@scohen scohen deleted the za-async-indexing branch September 19, 2023 18:36
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.

3 participants