Replace calls to panic! with Result types #98
Comments
|
Agreed, it'll be unfortunate that parsing will sometimes fail with regex compilation errors, but better than probably better than panicking. I'm unlikely to work on this myself any time soon though. |
|
Totally fine. Custom |
|
From discussion on #156 (comment): For checking that regexes compile, we might want to do that when we load the syntaxes from the yaml files (before linking them). For patterns with placeholders, we'd just use a dummy value ( This should not slow down normal use cases where loading from yaml happens once, is serialized to a binary dump, and then later that dump is loaded before matching. Loading the dump would not compile the regexes until they are needed, just like the current behavior. That gives us a much earlier (and more useful) check if there's a problematic regex in a syntax definition. Note that compiling could still fail at runtime if a dump from a previous version of syntect is loaded, and something about the regex engine changed in syntect, but that should be minimal compared to what we have now. And we could still make matching work with Results later. |
When Syntect fails to highlight a file it often does so by panicking. Most often these issues come from us making use of syntax definitions which simply aren't supported by Syntect very well (e.g. they use some new or obscure ST3 feature). There is an upstream issue to [return a `Result` type instead of panicking](trishume/syntect#98) when this occurs. Prior to this change, a user requesting syntect_server to highlight a bad file (i.e. hitting a case in the syntax definition not supported by Syntect) would result in `syntect_server` dying. This has been a known issue for a while, but in practice hasn't been that bad because these cases are relatively rare and Kubernetes / Docker restarts the process very quickly anyway. However, when it does occur it terminates all ongoing highlighting requests which causes blips that users see. After this change, we handle these panics by catching and unwinding the stack. This isn't perfect / ideal / idiomatic Rust code (see the `catch_unwind` docs), but it does solve the problem and is a better approach than e.g. adding more replicas of this service. Fixes sourcegraph/sourcegraph#3164
…20) When Syntect fails to highlight a file it often does so by panicking. Most often these issues come from us making use of syntax definitions which simply aren't supported by Syntect very well (e.g. they use some new or obscure ST3 feature). There is an upstream issue to [return a `Result` type instead of panicking](trishume/syntect#98) when this occurs. Prior to this change, a user requesting syntect_server to highlight a bad file (i.e. hitting a case in the syntax definition not supported by Syntect) would result in `syntect_server` dying. This has been a known issue for a while, but in practice hasn't been that bad because these cases are relatively rare and Kubernetes / Docker restarts the process very quickly anyway. However, when it does occur it terminates all ongoing highlighting requests which causes blips that users see. After this change, we handle these panics by catching and unwinding the stack. This isn't perfect / ideal / idiomatic Rust code (see the `catch_unwind` docs), but it does solve the problem and is a better approach than e.g. adding more replicas of this service. Fixes sourcegraph/sourcegraph#3164
This change makes syntect_server resilient to the two classes of problems we've seen in production usage of it: 1. Some specific language grammar/file pairs can cause syntect to panic internally. This is usually because syntect doesn't implement a specific sublime-syntax feature in some way and it [panics instead of returning result types](trishume/syntect#98). 2. Much rarer, some specific language/grammar file pairs can cause syntect to get stuck in an infinite loop internally -- never to return and consuming an entire CPU core until it is restarted manually. Previously we tried to solve #1 through stack unwinding (c5773da), but since the 2nd issue above also appeared it proved to not be sufficient on its own. It is still useful, though, because it can do per-request recovery of the first failure scenario above and as such it will be added back in. Even without stack unwinding, http-server-stabilizer helps both cases above by running and monitoring replicas of syntect_server. See the README in https://github.com/slimsag/http-server-stabilizer for details. It is important to note that all this does is stop these individual file failures from harming other requests to syntect_server. They are still issues on their own, and logging and Prometheus monitoring is now in place for us to identify when this is occurring and in which file it occurred so we can track down the issue and make small reproduction cases to file and fix upstream. Since only one instance of syntect_server was previously running and we now run multiple, more memory is needed. Each instance requires about 1.1 GB at peak (depending on which languages are used). The default is now to run 4 workers, so 4.4 GB is the minimum required and 6 GB is suggested. In the event only one worker is ran (via setting the env var `WORKERS=1`), stability is still greatly improved since the 2nd failure case above can only last a short period of time instead of until the container is restarted manually. Part of sourcegraph/sourcegraph#5406
* use http-server-stabilizer + 4 worker subprocesses This change makes syntect_server resilient to the two classes of problems we've seen in production usage of it: 1. Some specific language grammar/file pairs can cause syntect to panic internally. This is usually because syntect doesn't implement a specific sublime-syntax feature in some way and it [panics instead of returning result types](trishume/syntect#98). 2. Much rarer, some specific language/grammar file pairs can cause syntect to get stuck in an infinite loop internally -- never to return and consuming an entire CPU core until it is restarted manually. Previously we tried to solve #1 through stack unwinding (c5773da), but since the 2nd issue above also appeared it proved to not be sufficient on its own. It is still useful, though, because it can do per-request recovery of the first failure scenario above and as such it will be added back in. Even without stack unwinding, http-server-stabilizer helps both cases above by running and monitoring replicas of syntect_server. See the README in https://github.com/slimsag/http-server-stabilizer for details. It is important to note that all this does is stop these individual file failures from harming other requests to syntect_server. They are still issues on their own, and logging and Prometheus monitoring is now in place for us to identify when this is occurring and in which file it occurred so we can track down the issue and make small reproduction cases to file and fix upstream. Since only one instance of syntect_server was previously running and we now run multiple, more memory is needed. Each instance requires about 1.1 GB at peak (depending on which languages are used). The default is now to run 4 workers, so 4.4 GB is the minimum required and 6 GB is suggested. In the event only one worker is ran (via setting the env var `WORKERS=1`), stability is still greatly improved since the 2nd failure case above can only last a short period of time instead of until the container is restarted manually. Part of sourcegraph/sourcegraph#5406 * Add Dart+Kotlin support; upgrade to Rocket v0.4 * Continue serving requests when highlighting an individual file fails (#20)
|
I plan on looking into this for 5.0. This clippy command can be used to find potential panics: there are currently 38 of these (of which 5 are direct calls to |
|
Related: I didn't want to use the version 5 now and I had to fix some panics because it crashed broot, so I converted them to errors in this commit. This commit isn't directly usable as I had to start from an old commit but I don't think it would be hard to do the same on the whole up to date codebase. In order to fast release a new version of broot, I'm currently using a fork ( https://github.com/Canop/syntect ). Would there be interest in having a no-panic-4.6 branch hosted directly here to help waiting for the new 5.0 version ? Or would that just make the syntect project too messy ? |
|
@Canop Thanks for sharing! I will make sure to cover all cases of I will not use your API verbatim however. For example, I'm not a fan of the
If you want to embark on this journey together with me, I would appreciate your review on #424 which is one step towards resolving this issue. It sets the structure for future work on this issue by introducing a new enum |
|
I created a PR (#432) that removes all direct In the PR, all direct panics are gone, and regressions are prevented with a CI check. However, I have not taken care of replacing all |
|
I think we can consider this fixed by the 5.0.0 release. I don't know of any way to make syntect panic. There are probably such cases, but please open new specific issues with steps to reproduce and we can take care of that on a case by case basis. |
After doing a quick search, there are 8 occurrences of
panic!in syntect. It'd be great to have these errors bubbled up usingResulttypes with custom errors, instead. With the existing implementation, it's awkward to handle these, and because these methods don't signal the possibility of these errors, you won't realize there's an issue until you hit a runtime panic and have to work backwards to find the source.The text was updated successfully, but these errors were encountered: