-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
refactor(lsp): remove RwLock on Config
#13485
Conversation
) -> AsyncReturn<Result<Vec<serde_json::Value>, AnyError>>; | ||
uris: Vec<lsp::Url>, | ||
) -> AsyncReturn<Result<Vec<Result<SpecifierSettings, AnyError>>, AnyError>>; | ||
fn workspace_configuration(&self) -> AsyncReturn<Result<Value, AnyError>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having a low level configuration
method on the Client, I've split it up into two separate higher level functions. This simplifies the caller code and the REPL implementation of this trait.
@@ -236,149 +242,62 @@ pub struct Settings { | |||
#[derive(Debug)] | |||
pub struct Config { | |||
pub client_capabilities: ClientCapabilities, | |||
settings: Arc<RwLock<Settings>>, | |||
tx: mpsc::Sender<ConfigRequest>, | |||
settings: Settings, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RwLock
on this is removed along with the separate thread to call the client (it's now just fired off in a task and returns to use the same LanguageServer
mutex)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, ok... That whole tx/sender bit was to work around deadlocking issues I remember now, so removing it all together is a big 👍 .
scope_uri: None, | ||
section: Some(SETTINGS_SECTION.to_string()), | ||
}]) | ||
.await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the call that could cause a deadlock depending on how the client is implemented. Now this always happens outside the LanguageServer
mutex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM... and now I remember the whole "configuration server" stuff was to get around a deadlock issue way back when, so it is good to actually remove the RwLock all together... Thank you!
@@ -236,149 +242,62 @@ pub struct Settings { | |||
#[derive(Debug)] | |||
pub struct Config { | |||
pub client_capabilities: ClientCapabilities, | |||
settings: Arc<RwLock<Settings>>, | |||
tx: mpsc::Sender<ConfigRequest>, | |||
settings: Settings, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, ok... That whole tx/sender bit was to work around deadlocking issues I remember now, so removing it all together is a big 👍 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM too
Config
so the config state is behind the same lsp mutex as mostly everything elseMaybe fixes #13477.