Skip to content
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

Roll out Slang features in the VS Code extension for users #639

Closed
3 of 6 tasks
Xanewok opened this issue Nov 3, 2023 · 2 comments
Closed
3 of 6 tasks

Roll out Slang features in the VS Code extension for users #639

Xanewok opened this issue Nov 3, 2023 · 2 comments
Assignees

Comments

@Xanewok
Copy link
Contributor

Xanewok commented Nov 3, 2023

Blocking Changes

  • Upgrade server to latest version of Slang, as the existing version is too old (before add #[napi(catch_unwind)] everywhere #706).
  • Only report min/max supported versions in error here, as larger strings are truncated in sentry.
  • Investigate catching any remaining panics as explained in this comment, and see if there is anything else we should do.

Gradual Release

Other Important Optimizations

@OmarTawfik OmarTawfik self-assigned this Nov 8, 2023
@OmarTawfik OmarTawfik removed their assignment Feb 13, 2024
@OmarTawfik OmarTawfik changed the title Gradually roll out Slang features in the VS Code extension for users Roll out Slang features in the VS Code extension for users Feb 15, 2024
@Xanewok
Copy link
Contributor Author

Xanewok commented Mar 7, 2024

Some more contexts on catching the rogue Rust panics in the TS LSP server:

We expose our Rust code using napi-rs and we try to decorate every callable function with #[napi(catch_unwind)] precisely for that reason, to catch a Rust panic and expose it as an exception to the JS caller. The remaining issue is about catching remaining panics, which are panics in paths that were reached when not going through such decorated function, from JS.

Panicking in Rust is tricky.

There are two strategies: abort and unwind. We can't use the abort strategy, since we explicitly care about unwinding up to the FFI call and reporting the error, so we're left with unwinding.

Since we're loaded into a C++ code and there's no built-in Rust panic/C++ exception interop, we'd need to unwind ourselves and propagate a C++ exception up, but this means more manual catch_unwind and we're trying to handle the exact opposite case already. Moreover, unwinding out of Rust and crossing the FFI boundary is UB (when not aborting) and thus the native caller code would have to be very deliberate about Rust interop; I believe Node.js makes no explicit assumptions, so this is no-go.

There is extern "C" fn napi_fatal_error() that signals to the host that the process is about to abort, thus triggering emergency cleanup and the Node.js reporting when configured with --report-on-fatalerror, but there is no way to know that the Rust panic will most definitely be the one that is going to abort due to the fact it's uncaught. We could set our custom hook on the Rust side with std::panic::set_hook but that's executed for every panic, which includes the unwinding and about to be catched ones as well, and so aborting ourselves at the sight of a first panic defeats the purpose of catch_unwind and possibly handling them in general on the JS side.

One option is to use the custom panic hook and log every Rust panic to a report file and then handle those on abrupt LSP connection loss on the extension side, but in 99% of the time we hopefully will already catch any Rust panics via the #[napi(catch_unwind)]; we already use the logger to capture the errors using our telemetry setup.

As such, I don't think there's much value in doing that; if we get error reports about very rare/mysterious crashes I'd be open to revisiting it at some point but the scenario discussed seems all too theoretical for now. We're probably better off writing a napi proc macro wrapper that unconditionally tacks on catch_unwind to be safer and to trace more calls.

@OmarTawfik
Copy link
Collaborator

OmarTawfik commented Mar 15, 2024

Split the remaining work into smaller tasks. Closing this since all other work items are completed.
cc @Xanewok please let me know if you have any suggestions.

github-merge-queue bot pushed a commit that referenced this issue Apr 8, 2024
…nknown-linux-gnu` (#909)

Required for
NomicFoundation/hardhat-vscode#546 #639

Without this, we host-compiled our native addon with the GLIBC of the
runner (`ubuntu-22.04`), which is 2.33. That's too high and will cause
linking issues on older, stable distributions such as Debian 11 (2.31),
Debian 10 (2.28), Ubuntu 20.04 (2.31) etc.

The Linux requirement for VS Code atm is 2.28
(https://code.visualstudio.com/docs/supporting/requirements#_additional-linux-requirements)
and was bumped in [1.86](https://code.visualstudio.com/updates/v1_86).
Prior to that, the required version was 2.17 and we might consider
targeting it instead, until we bump the required VS Code engine for the
shipped extension (cc @kanej).

Here is the latest runs that check that the `infra publish npm
--dry-run` executes as expected and passes the relevant checks on our
CI:
https://github.com/Xanewok/slang/actions/runs/8472800732/job/23215682564.
The built artifacts are uploaded as part of the pipeline so they can be
additionally downloaded and inspected manually for the GLIBC symbols.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: ✅ Done
Development

No branches or pull requests

3 participants