Add Backtrace::set_enabled to override environment variables

[Nemo157]

Implements the API discussed in https://internals.rust-lang.org/t/backtrace-libstd-runtime-enable-disable/13268.

A couple of quotes for reasons why this should be configurable outside just the default environment variables:

When it could be needed? Production use on the server, when sys-admin detected incorrect behavior or developer want to detect why Error was returned. So it would be really useful to change backtrace behaviour of error at run-time.

• notice error in logs
• call private API (or debug button if UI app) and receive backtraces till disabled.

So the application user won't have the overhead of backtrace until it's required.

Another benefit is that you would be able to put this option into your app's configuration file (or any other unified source of configuration) instead of requiring an environment variable.

The reason this needs to be globally configurable is to support deeply enabling backtraces created by libraries, e.g. ones exposed through Error::backtrace. An application could use Backtrace::{force_capture, disabled} along with checking against its own enabled flag, but that would only control points at which it might capture backtraces itself, not backtraces that may be passed back from external libraries.

It might make sense to also expose Backtrace::enabled() publicly with this, that would avoid the cost of having to call Backtrace::capture().status() != BacktraceStatus::Disabled to detect the current state (though would not give any indication of whether it will be supported or not).

(Given that #72981 is close to merging, it's probably best to delay this till that is done and I can create a separate feature flag for it, just wanted to get discussion rolling already).

[rust-highfive]

r? @KodrAus

(rust_highfive has picked a reviewer for you, use r? to override)

[Nemo157]

cc @sfackler since you were against this on the internals discussion.

[KodrAus]

I don’t want to misrepresent @sfackler here, but it seems like they’re just trying to tease out alternatives rather than necessarily being against this.

In apps that I ship I just always have backtraces enabled. I would be interested to see a case where it’s important enough to capture backtraces to the point that you’d build infrastructure to control it remotely, but not enough to do so by default and avoid the need for that infrastructure entirely.

Having said that, this does seem pretty reasonable to me! Maybe we can land it, leave it unstable and see if it does find use.

[KodrAus]

Also cc @rust-lang/project-error-handling

[Nemo157]

One idea I had for a potential usecase that this doesn't support well: being able to enable backtraces for a specific request in a protocol like HTTP. You could imagine having a flag that is parsed early on and enables backtraces for the rest of the processing of a request. In a threaded or async server this would be somewhat complicated to pull off with this API (you would need a guard that does reference counting of how many requests currently want backtraces, and set/unset them globally based on that).

Having a thread-local flag (which could be turned into a task-local flag with existing async adaptors) would be more flexible for cases like this, but would need a more complicated design (probably a thread-local override, that can be cleared to fallback to the global setting).

The fact that this could be supported now, even with a somewhat complicated reference counting mechanism, means I think it's worth leaving that support up to a library.

[yaahc]

My initial reaction is that instead of having an override setting the entire enabled() function should be user customizable, in the same way set_hook is how you customize how panics are reported. This should make it possible for users to introduce as complex of logic as they need to, to support things like conditional capture based on the request type, for example.

[sfackler]

Yeah - I am not entirely opposed to doing this. I just generally feel bad about there only being this "big hammer" approach to backtraces being enabled. That being said, RUST_BACKTRACE is already a big hammer and this just provides a bit more flexibility there so I think it's fine.

Though a more configurable hook like @yaahc suggested would be preferable, but I'm not sure exactly what the hook would be able to work with for context.

[Dylan-DPC-zz]

@KodrAus any updates?

[KodrAus]

Let's put an unstable attribute on this, since we've got #72981 open to stabilize the core backtrace API.

[KodrAus]

I'm ok with merging this as unstable and shift conversation onto a tracking issue for a more complete design.

We should add a test at least that when we call set_enabled(false) that !Backtrace::enabled() and when we call set_enabled(true) that Backtrace::enabled().

[yaahc]

I think we might want to hold off on merging this for at least a little because I think it might conflict with the work I'm doing in #77384. In particular, one of the designs I'm investigating rn might supersede this eventually:

/// Global implementation of backtrace functinality. Called to create
/// `RawBacktrace` trait objects.
extern {
    #[linkage = "extern_weak"]
    #[link_name = "__rust_backtrace_impl"]
    static BACKTRACE_IMPL: *mut &'static dyn BacktraceImpl;
}

// perma(?)-unstable
#[unstable(feature = "core_backtrace", issue = "74465")]
pub trait BacktraceImpl: 'static {
    fn capture(&self) -> *mut dyn RawBacktrace;
    fn enabled(&self) -> bool;
}

// perma(?)-unstable
#[unstable(feature = "core_backtrace", issue = "74465")]
pub trait RawBacktrace: fmt::Debug + fmt::Display + 'static {
    unsafe fn drop_and_free(self: *mut Self);
}

#[unstable(feature = "core_backtrace", issue = "74465")]
pub struct Backtrace {
    inner: *mut dyn RawBacktrace,
}

Note: this is super WIP rn and just an example of the direction I'm going

[Nemo157]

I haven't had time to update it, so that's fine with me 😁. I've been watching #77384 so when that's looking a bit more stable I'll try rebasing over it and see how it affects this API.

[KodrAus]

I'll mark this one as blocked just so we know it's waiting on #77384

[bors]

☔ The latest upstream changes (presumably #92216) made this pull request unmergeable. Please resolve the merge conflicts.

[briansmith]

A couple of quotes for reasons why this should be configurable outside just the default environment variables:

There is another important reason: There is a consensus that env::set_var is not memory safe (in multi-threaded applications) and cannot be made so in general, so it is in the process of being deprecated. It seems it will be replaced with an unsafe function that does the same thing. Some applications currently use env::set_var("RUST_BACKTRACE", ...) to override the environment variable for backtraces, and they need a memory-safe replacement.

Importantly, they need a replacement that works not just for the library backtraces but also for backtraces for panics. In #90308 @Nemo157 wrote:

Actually, this only affects the library backtrace, the panic runtime probably does also need a in-code setter too.

I don't fully understand that comment, but it seems to mean that even if this PR is merged, the resultant API still wouldn't be sufficient for applications (like the ones I help maintain) to replace their env::set_var usage. I'm not sure if a separate issue should be filed about that, or whether we should amend this PR to deal with it.

[briansmith]

Back in November, 2020, @Nemo157 wrote:

(Given that #72981 is close to merging, it's probably best to delay this till that is done and I can create a separate feature flag for it, just wanted to get discussion rolling already).

and more recently @KodrAus wrote:

Let's put an unstable attribute on this, since we've got #72981 open to stabilize the core backtrace API.

Because of the env::set_var memory safety issue, I think that having an stable API to that can replace env::set_var("RUST_BACKTEACE", ...) is more urgent than the general backtrace API. And also, the backtrace API is no longer on the path to stablization, as on Jul 14, 2021 In #72981 (comment), @joshtriplett wrote:

Given that, we don't want to stabilize this [the backtrace API] at this time.

[briansmith]

This API doesn't look sufficient enough to handle the full functionality of what RUST_BACKTRACE can do, as RUST_BACKTRACE has multiple values, and might even be extended to more in the future. I think it would be good to at least change this to take an enum, something like this:

#[non_exhaustive]
pub Enum {
    Off,
    Short,
    Full,
}

Also, I think that this API should be "set at most once," similar to log::set_logger, as an application like the one I maintain doesn't want a (third-party) library to change this value after we set it in main(). (The ability to lock the backtrace level is another motivation for adding this new API.)

[joshtriplett]

@briansmith I don't think @rust-lang/libs-api would necessarily object to the narrow introduction of an API to configure the things that are otherwise configured via the environment, separately from the full backtrace feature.

It'll take some design, though.

[Mark-Simulacrum]

I've posted #93101 with a larger API surface and discussion of concerns/thoughts raised on this thread.

[Mark-Simulacrum]

I'm going to go ahead and close this in light of #93346 and the PR which made that tracking issue -- I think new experimentation in this space would want to take a fresh start based on the discussions there, and keeping this open then doesn't seem like it is the right step.