chore(lsp): fix possible race condition with tests expecting 3 publishDiagnostics messages, but getting 2 or 1 #12868
Conversation
…hDiagnostics messages The tests expect 3 publish notifications. It was possible for less than 3 to occur if two or more tasks set the diagnostics in the collection, exited the lock at the same time, then called `publish_diagnostics`
| let mut collection = collection.lock().await; | ||
| for diagnostic_record in diagnostics { | ||
| collection.set(DiagnosticSource::DenoLint, diagnostic_record); | ||
| } |
There was a problem hiding this comment.
Race condition was right here where the lock is released and then the lock was acquired in publish_diagnostics.
| } | ||
| publish_diagnostics(client, collection, &snapshot).await; | ||
| publish_diagnostics(client, &mut collection, &snapshot).await; |
There was a problem hiding this comment.
@kitsonk would it be better to pass the diagnostics directly to this function? It seems like they get set in the collection and then they're taken from there. Maybe they shouldn't be put in the collection if lint isn't enabled or the specifier isn't enabled?
There was a problem hiding this comment.
The considerations is that if you toggle a project as enabled or disabled (or for linting) the only way to clear diagnostics is to publish an empty set for each file.
There was a problem hiding this comment.
Ok, I'm not going to bother with this for now. I'll just fix this one issue in this PR.
|
I have seen some situations recently with the 1.16 LSP where the diagnostics are "behind" the current version of the document and never catch up. I am wondering if this racy condition was part of it, so I will be trying that out and if not, will investigate further. |
@kitsonk hmmm I'm not sure and don't think it would affect that. I was thinking some of the diagnostics code looks suspect, but I haven't looked at it in detail (only glanced at it). I think we could do something better here to improve it and eliminate that problem. Maybe I will look at it in more detail tomorrow. |
…hDiagnostics messages (#12868) The tests expect 3 publish notifications. It was possible for less than 3 to occur if two or more tasks set the diagnostics in the collection, exited the lock at the same time, then called `publish_diagnostics`
…hDiagnostics messages (denoland#12868) The tests expect 3 publish notifications. It was possible for less than 3 to occur if two or more tasks set the diagnostics in the collection, exited the lock at the same time, then called `publish_diagnostics`
The tests expect 3 publish notifications. It was possible for less than 3 to occur if two or more tasks set the diagnostics in the collection, exited the lock at the same time, then called
publish_diagnostics.