fix: Avoid UI freezes on Open Declaration with slow lang servers#1376
fix: Avoid UI freezes on Open Declaration with slow lang servers#1376sebthom merged 1 commit intoeclipse-lsp4e:mainfrom
Conversation
|
Thanks a lot for the PR, I think with this one, LSP4E should not cause UI freezes any longer. Great work! |
sebthom
force-pushed
the
async-open-declaration
branch
from
0ac9107 to
d99356c
Compare
Block the UI thread for max 200ms. If LS does not respond return DeferredOpenDeclarationHyperlink instance which asynchronously opens link once LS responded.
sebthom
force-pushed
the
async-open-declaration
branch
from
d99356c to
25c21e6
Compare
| private final long ttlNanos; | ||
|
|
||
| public DocumentOffsetAsyncCache(final Duration ttl) { | ||
| this.ttlNanos = TimeUnit.MILLISECONDS.toNanos(ttl.toMillis()); |
There was a problem hiding this comment.
why do we convert the time to nanos? Can we is millis not a good enough resolution, specially given that the input is millis?
There was a problem hiding this comment.
Duration calculations based on System.currentTimeMillis() is now considered bad practices as it is not time shift/leap second aware and can result in wrong calculations. Therefore one is supposted to use System.nanoTime() which returns nano seconds for measuring elapsed time.
| final Supplier<CompletableFuture<V>> supplier) { | ||
| // Fast path: return a completed future if a fresh value is already cached | ||
| final @Nullable V cachedNow = getNow(doc, offset); | ||
| if (cachedNow != null) |
There was a problem hiding this comment.
in this PR, ifs with only one statement do not have braces around the statement. I think LSP4E has the convention so far to always have braces, doesn't it? Can we have them here as well?
There was a problem hiding this comment.
AFAIK we do not use braces for single return statement. At least there are plenty of cases like this.
|
@sebthom , do you plan to do more fixes soon or shall I create a new release with this and your last improvement? I think they are both very nice improvements from the user point of view. |
@rubenporras I would actually appreciate a prompt release |
|
I will do so after I merge #1378 |
2 participants
Without the PR we currently wait up to 800ms which is for some LS too short (esp. when requests pile up) so the operation may consistently fail with:
Since the wait happens in the UI thread the timeout cannot be extended without making the UI freeze longer.
With this PR the UI thread will be blocked for max 200ms (or shorter if we agree on). If the LS does not respond in that time, a
DeferredOpenDeclarationHyperlinkinstance is returned which asynchronously opens the link once LS responded. Opening the link will be dismissed if the the editor was closed in the meantime, the document modified or the response took longer than 5 seconds.Addresses #97