This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Propogate context in LSPServer tests
AbandonedPublic

Authored by kadircet on May 3 2020, 3:15 AM.

Details

Reviewers
sammccall
Summary

This enables users to wait until async request completes. Taking a
response isn't enough, as receiving a response doesn't imply destruction of the
reqeust context.

This also fixes the raciness in lspserver latency metric test. As it needs to
wait for context destruction, rather than receiving a reply. Happy to perform
that fix in CallResult instead, i.e change LSPClient::call to perform the
insertion of Notification into context and change CallResult::take to wait for
destruction of context rather than receiving a reply(calling set).

Diff Detail

Event Timeline

kadircet created this revision.May 3 2020, 3:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2020, 3:15 AM

Hmm, I wonder if we should have a Context.bind(function) -> function.
I guess it runs into the usual thing of not being able to deduce a functor's signature, so you get an ugly templated return type..

clang-tools-extra/clangd/unittests/LSPClient.cpp
120

which waiters? isn't it just this thread?

122–125

nit: scope WithContext to exclude taking the lock?

kadircet marked 3 inline comments as done.May 3 2020, 6:18 AM
kadircet added inline comments.
clang-tools-extra/clangd/unittests/LSPClient.cpp
120

i was envisioning the future(thought this already had a blockUntilIdle :D)

kadircet updated this revision to Diff 261694.May 3 2020, 6:18 AM
kadircet marked an inline comment as done.
  • Address comments
sammccall added inline comments.May 3 2020, 10:43 AM
clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
164

Sorry, I didn't really put all the pieces together in my head the first time around.
The context propagation seems OK, but it's too fiddly as a way to control sequencing - can't you just call Client.sync() and assert after that?

kadircet updated this revision to Diff 261715.May 3 2020, 11:48 AM
  • Call sync instead of waiting on context destruction
kadircet updated this revision to Diff 261717.May 3 2020, 12:06 PM
  • Drop LSP latency test
kadircet abandoned this revision.May 4 2020, 6:15 AM

as discussed offline there is no real use case for this now and it is unclear whether there will be.

dropping it, until we've got a use case.