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

panic in Regex::regex() #68

Closed
drahnr opened this issue Apr 17, 2021 · 5 comments
Closed

panic in Regex::regex() #68

drahnr opened this issue Apr 17, 2021 · 5 comments

Comments

@drahnr
Copy link
Contributor

drahnr commented Apr 17, 2021

thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', /tmp/build/56ca5ece/git-pull-request-resource/../cargo/registry/src/github.com-1ecc6299db9ec823/nlprule-0.6.2/src/utils/regex.rs:78:33

thread 'stack backtrace:

<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', /tmp/build/56ca5ece/git-pull-request-resource/../cargo/registry/src/github.com-1ecc6299db9ec823/nlprule-0.6.2/src/utils/regex.rs:78:33

   0:     0x560ee78bdfd0 - std::backtrace_rs::backtrace::libunwind::trace::h5e9d00f0cdf4f57e

                               at /rustc/2fd73fabe469357a12c2c974c140f67e7cdd76d0/library/std/src/../../backtrace/src/backtrace/libunwind.rs:90:5

   1:     0x560ee78bdfd0 - std::backtrace_rs::backtrace::trace_unsynchronized::hd5302bd66215dab9

                               at /rustc/2fd73fabe469357a12c2c974c140f67e7cdd76d0/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5

There is an .unwrap on the regex.borrow() call which panics.

https://ci.spearow.io/teams/main/pipelines/cargo-spellcheck/jobs/pr-validate/builds/45

@drahnr
Copy link
Contributor Author

drahnr commented Apr 17, 2021

A bit of context:

Rules::suggest(...) is called in parallel over a shard immutable reference to a single &Rules instance. Looking into lazycell the ordering semantics look ok from the first glance, the API usage just seems incorrect (even in their example).

If thread A calls borrow() while thread B is just about to write the value to the UnsafeCell, it will be in LOCK state, which will return a None.
The only way I can think of doing this correctly is calling borrow() repeatedly in the else case until it succeeds.

@bminixhofer
Copy link
Owner

Hi, thanks for the issue. Refreshingly, something which I couldn't have foreseen :)

I believe the easiest fix is just to replace lazycell with once_cell, which is a more well used dependency anyway and has the get_or_init API which fits our use case.

I went ahead and implemented this: #69. Can you check if this solves the problem? Binaries from the latest release should still work.

@drahnr
Copy link
Contributor Author

drahnr commented Apr 17, 2021

Just had a peek into the once_cell crate, and there is a significant amount of non trivial synchronization logic happening, but I ran it a few times and it works consistently 🎉 - so #69 is good to go :) Thanks for the fast turnaround 👍

@drahnr
Copy link
Contributor Author

drahnr commented Apr 18, 2021

Could you craft a release, that's the last thing pending for a cargo-spellcheck 0.8.0 which is again needed for paritytech/parity-bridges-common#832

@bminixhofer
Copy link
Owner

Sure! Release CI is running for 0.6.3 now.

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

2 participants