search

benfred / py-spy

Sampling profiler for Python programs
MIT License
12.13k stars 401 forks source link

Assertion errors when #644

Open njsmith opened 6 months ago

njsmith commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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...