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

feat: overhaul the crash handler #10203

Merged
merged 35 commits into from
Apr 17, 2024
Merged

feat: overhaul the crash handler #10203

merged 35 commits into from
Apr 17, 2024

Conversation

paperdave
Copy link
Member

@paperdave paperdave commented Apr 12, 2024

This deletes the existing crash_handler.zig, panic_handler.zig, report.zig, and replaces it with a new crash handler which runs in all release builds. The primary goal of this is to allow all users to report useful crash reports to us using a "Trace String", a url-safe encoding of a stack trace or error return trace which can be demangled after the fact.

The tracestrings are all URLs from the base https://bun.report, where visiting the URL in a browser will redirect to a prompt to submit a GitHub issue, pre-filled with the remapped stack trace. Going to the homepage will show a frontend tool to just do the remapping.

Bun also adds --verbose-error-trace, which in platforms with error return traces enabled, these trace strings will be printed for any place we catch an error. This will help places we see users run into bun install errors but we don't know where it is at; We can ask them to run with the flag and share trace strings.

TODOs:

  • Implement tracestrings on all platforms
    • Windows
      • Encode
      • Parse
      • Remap
    • macOS
      • Encode
      • Parse
      • Remap
    • Linux
      • Encode (basically done, need to double-check it entirely works)
      • Parse
      • Remap
  • Deploy bun.report
    • Write remapping service
    • Fetching of debug information from bucket
    • GitHub redirect
    • View redirect
    • Frontend tool
    • Smart caching of already computed data
  • Verify that --watch reload-on-crash behavior works
    • On macOS
    • On Linux
    • On Windows
  • Make main returning an error a better experience.
  • Implement bun.handleErrorReturnTrace and --verbose-error-trace
  • Configure CI to store all debug symbols for all builds of bun.
  • Delete old dead implementations
  • Final pass over messages for trace strings. Currently uses a placeholder.
  • Revert changes into Bun.unsafe / change Bun.unsafe.segfault into Bun.unsafe.crash / remove that API entirely. Make Bun.unsafe.segfault private.
  • Verify multi-thread crashes work as expected
    • On macOS
    • On macOS --watch
    • On Linux
    • On Linux --watch
    • On Windows
    • On Windows --watch

Considering TODOs

  • Dump crash reports to file. This behavior was removed as it's previous form was not useful.
  • Get JS-stack trace information to identify non-user code. This wont be able to get source mapping though.

I might do some more breaking changes to the format of trace-strings before the main merge. That is what I'm most worried about for now.

Copy link
Contributor

github-actions bot commented Apr 12, 2024

Copy link
Contributor

github-actions bot commented Apr 12, 2024

Copy link
Contributor

github-actions bot commented Apr 12, 2024

@paperdave 2 files with test failures on bun-darwin-aarch64:

View test output

#54f47f47ec5f55328f11ceadcc1b7219b246ede7

Copy link
Contributor

github-actions bot commented Apr 12, 2024

@paperdave paperdave marked this pull request as ready for review April 17, 2024 07:28
src/bun.zig Outdated
@@ -1591,14 +1601,23 @@ pub fn reloadProcess(
const rc = bun.windows.TerminateProcess(@ptrFromInt(std.math.maxInt(usize)), win32.watcher_reload_exit);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const rc = bun.windows.TerminateProcess(@ptrFromInt(std.math.maxInt(usize)), win32.watcher_reload_exit);
const rc = bun.windows.TerminateProcess(bun.windows.GetCurrentProcess(), win32.watcher_reload_exit);

start_time = std.time.nanoTimestamp();
log_ = logger.Log.init(allocator);

var log = &log_;

var panicker = MainPanicHandler.init(log);
MainPanicHandler.Singleton = &panicker;
// var panicker = MainPanicHandler.init(log);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete

Copy link
Collaborator

@dylan-conway dylan-conway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments. Mostly wondering if there are four missing bun.handleErrorReturnTrace(err, @errorReturnTrace()); in run_command.zig

@Jarred-Sumner Jarred-Sumner merged commit c99d7ed into main Apr 17, 2024
16 of 22 checks passed
@Jarred-Sumner Jarred-Sumner deleted the dave/crash-handler branch April 17, 2024 22:32
@Jarred-Sumner Jarred-Sumner mentioned this pull request Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants