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

ledger: panic + poisoned mutex after new updated #128

Closed
naps62 opened this issue Nov 29, 2023 · 5 comments · Fixed by #129
Closed

ledger: panic + poisoned mutex after new updated #128

naps62 opened this issue Nov 29, 2023 · 5 comments · Fixed by #129

Comments

@naps62
Copy link

naps62 commented Nov 29, 2023

My app originally relied on my proposal (#125 ), which was closed in favor of a larger rewrite in the latest version.

I'm trying to run my app using this rewrite (by overriding the dependency to force ethers-rs to use it. since it's a minor version bump I assume no intended breaking changes?)

But any attempt to use it result in an immediate panic followed by PoisonError on every future attempt:

thread 'tokio-runtime-worker' panicked at /home/naps62/.cargo/git/checkouts/coins-a4bf2ba8095a65ef/9eff708/ledger/src/transports/native/hid.rs:143:42:
Error getting api_mutex: Hid(InitializationError)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'tokio-runtime-worker' panicked at /home/naps62/.cargo/git/checkouts/coins-a4bf2ba8095a65ef/9eff708/ledger/src/transports/native/hid.rs:142:47:
Could not lock api wrapper: PoisonError { .. }
thread 'tokio-runtime-worker' panicked at /home/naps62/.cargo/git/checkouts/coins-a4bf2ba8095a65ef/9eff708/ledger/src/transports/native/hid.rs:142:47:
Could not lock api wrapper: PoisonError { .. }
thread 'tokio-runtime-worker' panicked at /home/naps62/.cargo/git/checkouts/coins-a4bf2ba8095a65ef/9eff708/ledger/src/transports/native/hid.rs:142:47:
Could not lock api wrapper: PoisonError { .. }
thread 'tokio-runtime-worker' panicked at /home/naps62/.cargo/git/checkouts/coins-a4bf2ba8095a65ef/9eff708/ledger/src/transports/native/hid.rs:142:47:
Could not lock api wrapper: PoisonError { .. }
thread 'tokio-runtime-worker' panicked at /home/naps62/.cargo/git/checkouts/coins-a4bf2ba8095a65ef/9eff708/ledger/src/transports/native/hid.rs:142:47:
Could not lock api wrapper: PoisonError { .. }

One thing I should note, and that already happened even before all these fixes, is that I was already getting sporadic errors of the form hid_error is not implemented yet, which seems to come directly from hid.c, so unfortunately hard to debug.
I had these requests around a retry block, since they would randomly fail. But this new update means any failure leads to a poisoned mutex

@prestwich
Copy link
Member

is it possible to make a minimal repro? I've never encountered the hid_error is not implemented yet error.

by overriding the dependency to force ethers-rs to use it. since it's a minor version bump I assume no intended breaking changes

we're pre-1.0, so minor version bumps are still eligible for breaking changes. For this particular change, if you're using use coins_ledger::Ledger there should be no migration required. if you're using lower-level types, there may be breakage

@naps62
Copy link
Author

naps62 commented Nov 29, 2023

@prestwich here you go: https://github.com/naps62/coins_ledger-issue-128/tree/main

after narrowing this down, actually found it to be 2 different issues, though they're probably related. see the comments on main() for details.
this reproduces both issues consistently on my system (Manjaro Linux, Ledger Nano X)
tested with both 0.9.0 and 0.9.1

@prestwich
Copy link
Member

thanks! looking now :)

@prestwich prestwich linked a pull request Nov 29, 2023 that will close this issue
@prestwich
Copy link
Member

So this update makes it no longer panic. Instead a useful error message is added:
#129

Can you check that this resolves your issues? I ran it against your repo and it prevents panicking, although i'm not sure I completely understood your Issue 1

ffr, you can use the following in your cargo.toml to patch subdeps:

[patch.crates-io]
coins-ledger = { git = "...", tag = "..." }

https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html

@naps62
Copy link
Author

naps62 commented Nov 30, 2023

ffr, you can use the following in your cargo.toml to patch subdeps:

was wondering why you pointed this out, since I'm aware. just noticed I didn't properly override this dep on my repo above. was late at night, sorry 😄

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

Successfully merging a pull request may close this issue.

2 participants