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

Please make "backtraces" feature opt-in, not opt-out #113

Closed
lucab opened this issue Jul 13, 2019 · 6 comments · Fixed by #157
Closed

Please make "backtraces" feature opt-in, not opt-out #113

lucab opened this issue Jul 13, 2019 · 6 comments · Fixed by #157
Labels
breaking change Likely requires a SemVer version bump found in the field A user of SNAFU found this when trying to use it

Comments

@lucab
Copy link
Contributor

lucab commented Jul 13, 2019

Since #12 snafu supports capturing backtraces. While useful in several contexts, backtraces comes with additional costs both at runtime (#84) and at buildtime (it uses backtrace-sys under the hood, which brings in C source/compilation).

Backtraces are currently active by default and needs to be disabled (opt-out) by consumers via the default-features = false syntax.
In the context of an ecosystem of snafu-using libraries, this poses a problem when nesting a couple of levels of dependencies, as the top-level consumer is no more in charge of those features and can't directly opt-out.

Considering that cargo features are additive, a better approach would be to make backtraces disabled by default and let the final consumer enable them via a dedicated feature (opt-in).
That way any intermediate library could re-export the snafu/backtraces feature, and the top-level application/consumer would be the one (optionally) in charge of enabling backtraces across all crates.

@shepmaster shepmaster added breaking change Likely requires a SemVer version bump found in the field A user of SNAFU found this when trying to use it help wanted Extra attention is needed labels Jul 13, 2019
@shepmaster shepmaster changed the title cargo: please make "backtraces" feature opt-in, not opt-out Please make "backtraces" feature opt-in, not opt-out Jul 13, 2019
@shepmaster
Copy link
Owner

as the top-level consumer is no more in charge of those features

But the consumer is not actually in charge of this. The library creating the error already has to add a Backtrace type to each corresponding error variant:

#[derive(Snafu)]
enum Error {
    Something { backtrace: Backtrace }
}

With your proposed end result, every crate will also need to

  1. Add a feature flag for backtraces

  2. Add conditional compilation to each backtrace field:

    #[derive(Snafu)]
    enum Error {
        Something { #[cfg(my-backtraces)] backtrace: Backtrace }
    }

These multiple steps make me worried that library authors simply won't use backtraces at all, negating any benefits.

There's an inherent tension around default features. Without a feature by default, people get confused because "the docs show it's there, but it doesn't work for me". With a feature by default, time and space can be wasted on unused functionality.

Additionally, there's tension between library usage and application usage. Libraries should strive to be minimal in what they bring in to better work in more contexts, but applications tend to want to be maximal.

Anecdotally, library crates do a poor job of using default-features = false.

and can't directly opt-out.

Can you provide some concrete cases where such a thing would be beneficial? Presumably you are encountering one "in the wild" to prompt the request.

@shepmaster
Copy link
Owner

I wonder if we could do something like

#[cfg(backtrace)]
struct Backtrace(...);

#[cfg(not(backtrace))]
struct Backtrace;

This would lift the need for each library to add the conditional compilation. It might also allow them to avoid adding a feature flag, as the application author could be responsible for adding snafu = { features: [backtrace ] } to their Cargo.toml. Then the library author just has to ensure they add a Backtrace field where appropriate.

@shepmaster
Copy link
Owner

Another point is that eventually, backtraces will be added to the standard library. This will essentially make it "free" to use a backtrace.

@lucab
Copy link
Contributor Author

lucab commented Jul 13, 2019

Thanks for the very quick feedback!

I don't have a direct report from the field, as I'm still in the planning phase of migrating my existing crates (e.g. caps) to snafu, but I've encountered the same friction with both failures and error-chain.

I maintain a mix of both libraries and applications. For the former, it is painful to keep tracking and disabling default features, in order to let application developers being in control. For the latter, opt-ing out default features through chained dependencies becomes a game of whack-a-mole.

You suggestion of an inert Backtrace type makes sense to me. Additionally in that case, direct consumers should be able to enable your feature directly via --features snafu/backtraces.

@shepmaster
Copy link
Owner

General plan:

  • The backtraces feature is disabled by default
  • SNAFU unconditionally has a Backtrace type, encouraging error type authors to include it in their types.
  • When the application author enables the backtraces feature, the SNAFU Backtrace type starts to carry a backtrace::Backtrace.
  • The backtrace-crate feature implies backtraces and adds the AsRef implementation as shown in this PR. It wouldn't need to return an Option because of the requirement on backtraces.
  • In the future, we could add a backtrace-std feature that also implies backtraces. It would be incompatible with the backtrace-crate feature, but only an application should enable it so this shouldn't be a problem. At that point in time, we could also implement AsRef for the standard library Backtrace type.

For a library author, they just scatter Backtrace in all of their leaf errors (and backtrace(delegate) in aggregating errors, for now).

An application author adds features = [backtraces] and optionally one of backtrace-crate, backtrace-std (sometime), if they need to access the underlying type.

@lucab
Copy link
Contributor Author

lucab commented Aug 17, 2019

I didn't see opposition to this and the plan above seemed well-rounded, so I proceeded and opened a PR at #157.

@shepmaster shepmaster pinned this issue Aug 27, 2019
@shepmaster shepmaster unpinned this issue Nov 8, 2019
@shepmaster shepmaster removed the help wanted Extra attention is needed label Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Likely requires a SemVer version bump found in the field A user of SNAFU found this when trying to use it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants