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

Breaking change involving RefUnwindSafe in 1.1.3 #568

Closed
dtolnay opened this issue Mar 31, 2019 · 5 comments
Closed

Breaking change involving RefUnwindSafe in 1.1.3 #568

dtolnay opened this issue Mar 31, 2019 · 5 comments

Comments

@dtolnay
Copy link
Member

dtolnay commented Mar 31, 2019

The following code compiles against regex 1.1.2 but not 1.1.3:

fn f(r: regex::Regex) {
    let _ = std::panic::catch_unwind(|| r);
}
error[E0277]: the type `(dyn aho_corasick::prefilter::Prefilter + 'static)` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
 --> src/main.rs:2:13
  |
2 |     let _ = std::panic::catch_unwind(|| r);
  |             ^^^^^^^^^^^^^^^^^^^^^^^^ `(dyn aho_corasick::prefilter::Prefilter + 'static)` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
  |
  = help: within `regex::exec::ExecReadOnly`, the trait `std::panic::RefUnwindSafe` is not implemented for `(dyn aho_corasick::prefilter::Prefilter + 'static)`
  = note: required because it appears within the type `*const (dyn aho_corasick::prefilter::Prefilter + 'static)`
  = note: required because it appears within the type `std::ptr::Unique<(dyn aho_corasick::prefilter::Prefilter + 'static)>`
  = note: required because it appears within the type `std::boxed::Box<(dyn aho_corasick::prefilter::Prefilter + 'static)>`
  = note: required because it appears within the type `aho_corasick::prefilter::PrefilterObj`
  = note: required because it appears within the type `std::option::Option<aho_corasick::prefilter::PrefilterObj>`
  = note: required because it appears within the type `aho_corasick::nfa::NFA<u32>`
  = note: required because it appears within the type `aho_corasick::ahocorasick::Imp<u32>`
  = note: required because it appears within the type `aho_corasick::ahocorasick::AhoCorasick<u32>`
  = note: required because it appears within the type `std::option::Option<aho_corasick::ahocorasick::AhoCorasick<u32>>`
  = note: required because it appears within the type `regex::exec::ExecReadOnly`
  = note: required because of the requirements on the impl of `std::panic::UnwindSafe` for `std::sync::Arc<regex::exec::ExecReadOnly>`
  = note: required because it appears within the type `regex::exec::Exec`
  = note: required because it appears within the type `regex::re_unicode::Regex`
  = note: required because it appears within the type `[closure@src/main.rs:2:38: 2:42 r:regex::re_unicode::Regex]`
  = note: required by `std::panic::catch_unwind`

I noticed that this is breaking rustfmt (https://travis-ci.org/dtolnay/syn/jobs/513667990). Is there an easy fix on your end or would it be better for us to work around this downstream? Thanks!

BurntSushi added a commit to BurntSushi/aho-corasick that referenced this issue Mar 31, 2019
It appears that using a trait object internally implies that the containing
type cannot be assumed to be UnwindSafe. This wasn't previously an
explicit API guarantee, but the AhoCorasick automaton in 0.6.x was
UnwindSafe and it's probably a good idea to keep that particular feature.
Here, we satisfy that by requiring Prefilter to be UnwindSafe, so that all
Prefilter trait objects are therefore also UnwindSafe.

In particular, this introduced a regression in regex that caused a regex
to not be UnwindSafe because it contained an AhoCorasick automaton.
See rust-lang/regex#568 for details.
@BurntSushi
Copy link
Member

BurntSushi commented Mar 31, 2019

@dtolnay Thanks for the bug report! This was definitely not intentional. I've fixed this in regex 1.1.4. (More specifically, I fixed it in aho-corasick 0.7.4, and regex 1.1.4 now requires aho-corasick 0.7.4.)

I'll confess that the UnwindSafe marker trait hasn't been something that I've been keeping a keen eye on. In particular, it seems incredibly easy to foul this up whenever one uses trait objects. But I guess it's the same situation as with Send and Sync. I think I've just gotten used to that, but haven't been thinking about UnwindSafe.

In this particular circumstance, I fixed it by just making Prefilter: RefUnwindSafe + UnwindSafe because none of the implementations actually use interior mutability (and I don't think they ever will). You can see my fixes here BurntSushi/aho-corasick@df7fee7 and here BurntSushi/aho-corasick@5f220e6

Advice is welcome if you think there's anything else I should be doing! (I have now added marker trait tests on regex's public API, so this particular regression shouldn't happen again.)

@dtolnay
Copy link
Member Author

dtolnay commented Mar 31, 2019

Thanks for the quick fix! I was unfamiliar with this failure mode too. I guess it is not so common to add new trait objects into a struct and not so common to use catch_unwind, so they don't intersect much.

@Turbo87
Copy link
Member

Turbo87 commented Apr 1, 2019

@BurntSushi https://github.com/rust-lang/regex/blob/1.1.4/Cargo.toml#L29

looks like regex 1.1.4 is still depending on aho-corasick 0.7.3 🤔

Turbo87/ogn-web-gateway#139 seems to be breaking due to that

@BurntSushi
Copy link
Member

@Turbo87 No, there was a typo in my comment above. aho-corasick 0.7.3 is the latest release and should have all relevant fixes.

Your build is breaking because of an unrelated reason. Compare the output of your build to the error reported in this ticket, for example. I filed a separate bug here: #570 (Which is now fixed in regex 1.1.5.)

@Turbo87
Copy link
Member

Turbo87 commented Apr 1, 2019

I see. thanks for the quick response! ❤️

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

No branches or pull requests

3 participants