Garbage collection causes panic when accessing ParsedDeclId from ParsedDeclEngine
#5531
Assignees
Labels
bug
Something isn't working
compiler: frontend
Everything to do with type checking, control flow analysis, and everything between parsing and IRgen
language server
LSP server
Comments
JoshuaBatty
added
bug
JoshuaBatty
changed the title
ParsedDeclEngineParsedDeclId from ParsedDeclEngine
|
So, I was easily able to reproduce this with your branch. Been investigating why this happens, still not 100% sure, but I cannot reproduce it with the random waits removed. That makes me suspect this might be due to a concurrency issue. Do you think that could be the case? With the current LSP architecture, can we somehow be clearing the parsed decl engine in one task/thread, while another is traversing the parsed tree? |
|
Yeah i think you might be correct, I commented out clearing the parsed_decl_engine and managed to also hit a similar panic with the type engine indexing OOB. 
Makes sense why it wasn't crashing without the random wait times removed as compilation is cancelled before it has a chance to start otherwise. There is only a single compilation thread so it's not obvious to me how the engines could be a concurrency issue but I'll see if I can dig a bit deeper into that thread now. |
|
Ok have dug in a bit further here. below is the output of a typical successful compilation But here is the output when it fails Notice how when successful we get @tritao i've pushed another commit to my branch that has these new debug prints. |
|
Ok, I think I found the culprit but I don't understand why it's happening. If I removed the impl Clone for QueryEngine {
fn clone(&self) -> Self {
Self {
parse_module_cache: RwLock::new(self.parse_module_cache.read().unwrap().clone()),
programs_cache: RwLock::new(self.programs_cache.read().unwrap().clone()),
}
}
} |
|
I've reverted the optimization in #5548 for now. We should still try and get the optimization back in once we understand how to avoid this crash from happening. |
JoshuaBatty
added a commit
that referenced
this issue
Feb 5, 2024
## Description This was added as an optimization in #5472. However, it seems to be the cause of a transient bug that is causing the language server to crash outlined in #5531. We should revert this optimization until we understand how to avoid this crash from happening. cc @tritao ## Checklist - [x] I have linked to any relevant issues. - [ ] I have commented my code, particularly in hard-to-understand areas. - [ ] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [ ] I have added tests that prove my fix is effective or that my feature works. - [ ] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [ ] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [ ] I have requested a review from the relevant team or maintainers.
|
This should all be resolved now as of #5813 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Today I noticed a panic when using the language server. It only seemed to happen very rarely so I wrote the below test.
Also, running the test with a single thread like below seemed to trigger the error more frequently.
And below is the output that caught the panic with the backtrace. It seems that in this instance it took 3243 didChange events for the
ParsedDeclEngineto not contain an assumed validParsedDeclIdfor thelet struct_decl = ctx.engines.pe().get_struct(self);to work.I don't understand why calling GC the thousands of times before hand didn't lead to this
ParsedDeclIdnot being removed. @tritao do you have any suggestions on what might be happening here? I have a branch here if you want to try recreating locally.The text was updated successfully, but these errors were encountered: