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

Don't blacklist files forever in the lsp #1973

Merged
merged 1 commit into from
Jun 21, 2024
Merged

Conversation

thufschmitt
Copy link
Contributor

When the evaluator in the LSP times out or overflows its stack, the faulty file is blacklisted so that it doesn't get evaluated any more. This blacklist used to be permanent, which is generally not desirable since it makes the LSP unusable.

Change that to only blacklist the file for a fixed delay currently (hardcoded at 30s).

I didn't add a test, because I'm not sure how to do that properly (I assume that a test that waits for 30s isn't very desirable).
Any hint at how I could do that is welcome.

When the evaluator in the LSP times out or overflows its stack, the
faulty file is blacklisted so that it doesn't get evaluated any more.
This blacklist used to be permanent, which is generally not desirable
since it makes the LSP unusable.

Change that to only blacklist the file for a fixed delay currently
(hardcoded at 30s).
@github-actions github-actions bot temporarily deployed to pull request June 21, 2024 08:16 Inactive
@yannham yannham requested a review from jneem June 21, 2024 08:20
Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

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

LGTM. I honestly have no idea if the timeout is reasonable; practice will tell, but you're the one hitting the issue, so you probably know better.

For tests, I imagine one solution would be to make the blacklist duration some kind of parameters instead of a static constant, that could set to a reasonable value in tests. Though we don't really have any form of config right now for the LSP, so that would probably lead to a larger change.

Given the featured patch is really simple, at that you'll be experimenting with it, for now I'm ok with the absence of a test.

I'm letting @jneem take a look as well.

@thufschmitt
Copy link
Contributor Author

one solution would be to make the blacklist duration some kind of parameters instead of a static constant, that could set to a reasonable value in tests. Though we don't really have any form of config right now for the LSP, so that would probably lead to a larger change.

I though about that indeed (also for the eval timeout that might need to be bumped). Maybe a set of environment variables (like NICKEL_LSP_EVAL_TIMEOUT_SECONDS) could be a decent poor-man solution?

@yannham
Copy link
Member

yannham commented Jun 21, 2024

I think I would prefer a proper config solution - a value that is stored and accessible in the LSP somewhere in a structure (then it can be filled from an environment variable, but it's not necessarily simpler than a CL arg, and in fact we can have both such as for NICKEL_IMPORT). The testing motivation doesn't seem important enough to mandate a temporary poor-man solution, IMHO (and I suspect the full solution isn't very hard either).

@thufschmitt
Copy link
Contributor Author

Well, I got a bit carried away: #1974

@yannham yannham added this pull request to the merge queue Jun 21, 2024
Merged via the queue into master with commit e2e6742 Jun 21, 2024
5 checks passed
@yannham yannham deleted the lsp-blacklist-timeout branch June 21, 2024 17:16
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