From 66b66de96a78af6162e581c2986e3ef4ec8733c4 Mon Sep 17 00:00:00 2001
From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com>
Date: Wed, 1 May 2024 20:31:11 -0700
Subject: [PATCH] fix(lsp): Catch cancellation exceptions thrown by TSC, stop
 waiting for TS result upon cancellation (#23645)

Fixes #23643.

We weren't catching the cancellation exception thrown by TSC on the JS
side, so the rust side was catching this exception and then attempting
to print out the exception via `toString`. That last bit resulted in a
cryptic `[object Object]` showing up in the logs like so:

```
Error during TS request "getCompletionEntryDetails":
  [object Object]
```

I'm not 100% sure how we weren't seeing this in the past. My guess is
that #23409 and the subsequent PR to improve the exception catching and
logging surfaced this, but I'm still not quite clear on it.

My initial fix here returned `null` to rust when a server request was
cancelled, but this resulted in a deserialization error when we
attempted to deserialize that into the expected response type. So now,
as soon as the request's cancellation token signals we'll stop waiting
for a response and return an error (which will get swallowed as the LSP
request is being cancelled).

I was a bit surprised to find that [this
branch](https://github.com/nathanwhit/deno/blob/0c671c9792ac706c1ecd60f88efdc5eb8e941917/cli/lsp/tsc.rs#L1093)
actually executes sometimes, I believe due to the fact that aborting a
future may not [immediately stop its
execution](https://docs.rs/futures/latest/futures/stream/struct.AbortHandle.html#method.abort).
---
 cli/lsp/tsc.rs              | 26 +++++++++++++++++++++-----
 cli/tsc/99_main_compiler.js | 27 ++++++++++++++++++++++++---
 2 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs
index 66e834bc28..606e47d72c 100644
--- a/cli/lsp/tsc.rs
+++ b/cli/lsp/tsc.rs
@@ -1074,18 +1074,25 @@ impl TsServer {
     }
     let token = token.child_token();
     let droppable_token = DroppableToken(token.clone());
-    let (tx, rx) = oneshot::channel::<Result<String, AnyError>>();
+    let (tx, mut rx) = oneshot::channel::<Result<String, AnyError>>();
     let change = self.pending_change.lock().take();
     if self
       .sender
-      .send((req, snapshot, tx, token, change))
+      .send((req, snapshot, tx, token.clone(), change))
       .is_err()
     {
       return Err(anyhow!("failed to send request to tsc thread"));
     }
-    let value = rx.await??;
-    drop(droppable_token);
-    Ok(serde_json::from_str(&value)?)
+    tokio::select! {
+      value = &mut rx => {
+        let value = value??;
+        drop(droppable_token);
+        Ok(serde_json::from_str(&value)?)
+      }
+      _ = token.cancelled() => {
+        Err(anyhow!("request cancelled"))
+      }
+    }
   }
 }
 
@@ -4276,6 +4283,15 @@ impl TscRuntime {
             "Error during TS request \"{method}\":\n  {}",
             stack_trace.to_rust_string_lossy(tc_scope),
           );
+        } else if let Some(message) = tc_scope.message() {
+          lsp_warn!(
+            "Error during TS request \"{method}\":\n  {}\n  {}",
+            message.get(tc_scope).to_rust_string_lossy(tc_scope),
+            tc_scope
+              .exception()
+              .map(|exc| exc.to_rust_string_lossy(tc_scope))
+              .unwrap_or_default()
+          );
         } else {
           lsp_warn!(
             "Error during TS request \"{method}\":\n  {}",
diff --git a/cli/tsc/99_main_compiler.js b/cli/tsc/99_main_compiler.js
index e5b51cab0b..ddbd77ae0e 100644
--- a/cli/tsc/99_main_compiler.js
+++ b/cli/tsc/99_main_compiler.js
@@ -1053,6 +1053,15 @@ delete Object.prototype.__proto__;
     debug("<<< exec stop");
   }
 
+  /**
+   * @param {any} e
+   * @returns {e is (OperationCanceledError | ts.OperationCanceledException)}
+   */
+  function isCancellationError(e) {
+    return e instanceof OperationCanceledError ||
+      e instanceof ts.OperationCanceledException;
+  }
+
   function getAssets() {
     /** @type {{ specifier: string; text: string; }[]} */
     const assets = [];
@@ -1149,14 +1158,14 @@ delete Object.prototype.__proto__;
           return respond(id, diagnosticMap);
         } catch (e) {
           if (
-            !(e instanceof OperationCanceledError ||
-              e instanceof ts.OperationCanceledException)
+            !isCancellationError(e)
           ) {
             if ("stack" in e) {
               error(e.stack);
             } else {
               error(e);
             }
+            throw e;
           }
           return respond(id, {});
         }
@@ -1168,7 +1177,19 @@ delete Object.prototype.__proto__;
           if (method == "getCompletionEntryDetails") {
             args[4] ??= undefined;
           }
-          return respond(id, languageService[method](...args));
+          try {
+            return respond(id, languageService[method](...args));
+          } catch (e) {
+            if (!isCancellationError(e)) {
+              if ("stack" in e) {
+                error(e.stack);
+              } else {
+                error(e);
+              }
+              throw e;
+            }
+            return respond(id);
+          }
         }
         throw new TypeError(
           // @ts-ignore exhausted case statement sets type to never