Support configuring whether to capture backtraces at runtime by Mark-Simulacrum · Pull Request #93101 · rust-lang/rust (original) (raw)

I agree that exposing the internals of the default hook in a way that users can customize seems like a good goal. However, I don't think it precludes providing the APIs proposed here. In fact, I think rather than "building up" the default hook by providing a series of options that it takes, we should strive to expose these as global flags that std provides read/write APIs for (this PR provides the write side).

That way a downstream library that wants to expose similar functionality to a panic hook (e.g., interfacing with Python or similar, and wanting the same behavior from exceptions thrown in Python as panics thrown in Rust) can read those options and respect the user's choices -- without needing to be separately configured. I suspect many of the configuration knobs we might want to provide will have similar applications outside just that of the std hook.

Fair enough, are you thinking of adding get_library and get_panic functions that return the set CapturePreference? Either way big +1 for the style of approach of exposing the building blocks rather than a Builder API.

This is all to say that while I agree that we should plan for -- and expect -- that users will want to customize the panic hook and other error handling functionality, I am not convinced that the right mechanism for doing so is to leverage a builder-style API that builds up a panic hook implementation. And, it seems to me, that even if we had such a builder style API, the APIs added by this PR would not be an inconvenience, but rather complement it relatively well. At minimum, the set_library function is largely orthogonal from the panic hook, so would presumably want to exist regardless.

Fair point fair point, I didn't really intend to push the builder API specifically, just that I want some uniformity between how we interface with Backtrace across both panics and recoverable errors and to make it easy to preserve this functionality when transitioning from the default panic hook to a customized one.

Regarding the specific split proposed here, one of the things that bothers me and feels inconsistent is that Backtrace:capture() is influenced by set_library, but there is no equivalent public API for capturing a backtrace for a panic that is influenced by set_panic. I'm imagining a situation where we have both the set and get functions for these settings, and it feels kinda strange to respect the set_library configuration by calling Backtrace::capture, but to respect the set_panic configuration by calling if get_panic() == CapturePreference::??? { Backtrace::force_capture() }, also, I'm not really sure how one would respect the Full/Short split in this case.

I'm thinking this implies that the Backtrace apis should really be something like this:

fn set_library(CapturePreference); fn get_library() -> CapturePreference; fn capture_library() -> Backtrace; fn force_capture_library() -> Backtrace;

fn set_panic(CapturePreference); fn get_panic() -> CapturePreference; fn capture_panic() -> Backtrace; fn force_capture_panic() -> Backtrace;

Then you can respect or override either configuration while respecting the format configuration of each knob. Though I guess this raises the question, do we expect users to ever enable both kinds of backtraces but have one be Full and the other Short?

If not we could make the format a separate knob and make force_capture generally usable for both sides. Something like this:

fn set_format(DisplayFormat); fn get_format() -> DisplayFormat;

fn set_library(enabled: bool); fn get_library() -> bool; fn capture_library() -> Backtrace;

fn set_panic(enabled: bool); fn get_panic() -> bool; fn capture_panic() -> Backtrace;

fn force_capture() -> Backtrace;

And as a minor nit, and I know you already mentioned this in the top level comment so this isn't a blocker just adding support behind that unresolved question: I really don't like the set_library/set_panic names, though I guess we're kinda stuck with it based on the naming of the already stable RUST_BACKTRACE and RUST_LIB_BACKTRACE env variables... It just feels lame calling the backtrace that's being used for recoverable errors a library backtrace. "library" seems like an irrelevant distinction here. And set_panic makes it seem like its somehow related to the panic APIs, rather than just being intended for use in panic reporting implementations by convention. panic vs library feels like a poor dichotomy. I kinda wish the split was recoverable vs non-recoverable, or user-facing vs dev-facing, or really just anything more direct.

edit: wait actually no, RUST_LIB_BACKTRACE is not stabilized, we're not stuck here!