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

Assertion errors when #644

Open
njsmith opened this issue Dec 15, 2023 · 2 comments
Open

Assertion errors when #644

njsmith opened this issue Dec 15, 2023 · 2 comments

Comments

@njsmith
Copy link

njsmith commented Dec 15, 2023

Since upgrading to py-spy 0.3.14, we're seeing lots of messages like:

thread '<unnamed>' panicked at 'called `Result::unwrap_err()` on an `Ok` value: ()', src/sampler.rs:311:49

That assertion appears to have been added in #623 "fix lint errors": https://github.com/benfred/py-spy/pull/623/files#diff-581fb0889c2c34ba74526f0386e08ef0f443dfe1dd46ac2f04a6f1fc17298998L59-R311

I'm not sure what this code is doing, but the changes look suspicious -- the old code did if thing.is_err() {}, ie explicitly saying we don't care whether thing succeeds or fails. The new code does thing.unwrap_err(), which is an assertion that thing must always fail: https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_err

I suspect this should be either .unwrap() to assert that it succeeds, or else let _ = thing; if we want to idiomatically discard errors.

benfred added a commit that referenced this issue Dec 16, 2023
We were calling unwrap_err when failing to create a PythonSpy
object in sampler.rs (on the result of sending the error on
a channel), when we should have been calling unwrap. Fix.

See #644
@benfred
Copy link
Owner

benfred commented Dec 16, 2023

Thats an excellent callout! that line shouldn't be unwrap_err - and should be unwrap instead. I've pushed out a fix in #645

This is in error handling code though, so I don't think this won't fix the error - but will just lead to the error being reported successfully (like py-spy won't panic after this fix, but will instead just report the actual error that is causing initialization to fail =(. If you enable logging is anything shown ? (like RUST_LOG=info py-spy record -p PID ?)

This code is also unreleased - which means I both really appreciate you catching this bug before it gets pushed out more widely, but also that I can't see it being related to upgrading to 0.3.14 (since 0.3.14 doesn't contain that commit).

@njsmith
Copy link
Author

njsmith commented Dec 16, 2023

Yeah my colleague just pointed that I was confused, and while our py-spy --version is saying 0.3.14, our recent upgrade was actually to a py-spy git commit to pick up the py311 fix in #635. So that part makes sense at least!

Good point about this being an error path though -- on my quick skim I thought we went through here on individual attempts to attach, so it could just mean python was still starting up. Going to see if the improved error reporting makes anything clearer...

benfred added a commit that referenced this issue Dec 16, 2023
We were calling unwrap_err when failing to create a PythonSpy
object in sampler.rs (on the result of sending the error on
a channel), when we should have been calling unwrap. Fix.

See #644

Also fix freebsd ci
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