-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix: inlay hints config desyncronization between the frontend and the backend #3948
Conversation
editors/code/src/inlay_hints.ts
Outdated
if ( | ||
!ctx.config.inlayHints.typeHints && | ||
!ctx.config.inlayHints.parameterHints && | ||
!ctx.config.inlayHints.chainingHints | ||
) { | ||
return this.dispose(); | ||
} | ||
await sleep(10); |
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.
let's bump this to 100, as this depends not only on the event loop in the extension itself, but on the event loop in the server. But also, urgh, causality broken once again!
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.
(caution: noob rust-analyzer user, excuse dumb questions for now!)
Is it possible to have the server keep causality in case of config update event specifically?
As in, everything sent after config update event, executes after the event?
Perhaps the server can block all tasks on receiving config update notification, to ensure no task can "race" config update.
Otherwise I feel this issue of broken causality will happen in other parts of configuration updates, not just inlay_hints.ts.
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.
Actually, I think we can handle this correctly without major changes.
We should add a middlewhere for getConfigurationRequest
, and schedule an update after we've responded.
Probably need a didChangeServerConfig
event for this
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.
@matklad I've already tried the middleware approach and it failed, because the middleware returns the response to the server, but you never know when this response is delivered and when the server has applied the update
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.
Actaully, blocking while the config update is in progress as proposed by @nokola seems like an approach that guarantees consistency, since the server will never use the out-of-date config resonding for inlay hints.
This whole problem appeared because we did our custom implementation of inlay hints, it might appear if we implement anything custom like inlay hints in future
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.
I feel strongly that the server should just process requests and responses in-order, as they arrive. This could be enough to ensure causality, if the client cooperates.
Holding of responses is much harder mental model, and it deadlocks in the workstace, where the underlying request queue is filled.
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.
@matklad So there might be the problem with the order of processing on the server, because the middleware approach doesn't work for me, the inlay hints request somehow is processed before the config update response. I may have done some mistake, let me double check and maybe create another PR for that approach
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.
@matklad what do you mean by “holding of responses” mental model?
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.
Never mind, I re-read the thread, I assume you meant "server does not start another request until current one is finished".
I assume the server starts processing requests in order but does not finish processing requests in order.
It shouldn't matter: server processes requests in order, so it is safe to
re-query hints after we've send the reply, we don't have to wait until the
reply is acknowledged
…On Sat, 11 Apr 2020 at 18:18, Veetaha ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In editors/code/src/inlay_hints.ts
<#3948 (comment)>
:
> if (
!ctx.config.inlayHints.typeHints &&
!ctx.config.inlayHints.parameterHints &&
!ctx.config.inlayHints.chainingHints
) {
return this.dispose();
}
+ await sleep(10);
@matklad <https://github.com/matklad> I've already tried the middleware
approach and it failed, because the middleware returns the response to the
server, but you never know when this response is delivered and when the
server has applied the update
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3948 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANB3M4NVUBC342KJUD2ORTRMCJ3PANCNFSM4MGBFIZQ>
.
|
Yeah @matklad as I said, the approach failed, not caring about waiting for the config response acknowledged the inlay hints somehow do get processed with out-of-date config, so this means we still need to add a |
… backend See the explanation in the issue comment: rust-lang#3924 (comment)
8c58c38
to
7534266
Compare
One more option that came to my mind is using an optimistic concurrency model, where the inlays request supplies the config version and if that one mismatches with the one stored in the language server we do the retry and this naturally fits into |
Another option: send the config (or config updates only) with the config change notification. Assuming also keep the pull model. btw, was the model "push" before then changed to "pull" for config - curious what the discussion was about it. |
I do think that the push-based model would be the best fit here. The protocol developers deprecated it in favor of pull-based, but what it seems to me, there is no overwhelmingly greater number of pros for neither model, there should be an option to use any of them even in parallel. Sending the config with the notification itself sounds like it can fix this, but the |
The crazy option is removing one source of truth and passing the inlays configuration with the request itself as parameters (this is only a number and 3 booleans, that can be further compressed). |
Lets r+ As I don't think it's really worth the time to get to the bottom of this, in contrast to, for example, working on the upstream implementation.
This... actually is really insightful. That's a model at which we've arrived internally: #3551. Really, the relevant config should just be passed in together with the relevant requests, as opposed to be some global state which the client manipulates in a round-about way. The problem is, LSP absolutely does not work that way. |
bors r+ |
Build succeeded: |
See the explanation in the issue comment:
#3924 (comment)
Workaround-ly fixes: #3924