panic(nil) not handled properly

[rittneje]

Prior to Go 1.21, recover could return nil in two cases:

1. if there was no panic
2. if you explicitly did panic(nil)

Therefore, it was not possible to distinguish the two.

Now (barring GODEBUG=panicnil=1), explicitly calling panic(nil) will cause recover to return runtime.PanicNilError, which means the two case are distinguishable.

In this light, I don't understand the intent of the changes in #960. Now this library now goes out of its way to treat the two cases the same again. In particular, it will treat runtime.PanicNilError as if it didn't panic.

I would expect panic(nil) to be treated the same as any other panic, which was the motivation for the change in Go 1.21. Why is runtime.PanicNilError treated specially by this library?

[iamemilio]

This seems like this may have been caused by a misinterpretation of some legacy code. A panic is a panic, even if its value is nil. We will address this issue in an upcoming relese.

[iamemilio]

Ok, after assessing the internal tests, it does seem like this project historically did avoid collecting nil errors. This may change the behavior of the app in user's environments. I'm going to label this as a feature request for now, and we will figure out how to move forward from there.

[rittneje]

@iamemilio The current behavior is a bug. If I do panic(nil) without the agent injected, I properly get runtime.PanicNilError from recover, but now I get nil because this library incorrectly decides not to re-panic. Use of this library should never change the behavior of my code.

I also don't know how not collecting nil errors is relevant. As I mentioned, previously it was not possible to tell the difference between "no panic" and panic(nil). So this library (reasonably) assumed the former. But now it is clearly getting a non-nil error in the form of runtime.PanicNilError.

Lastly, while it is true that panic(nil) resulting in runtime.PanicNilError is a behavioral change, that change is due to the Go runtime itself, not this library. If the user has not opted out of the new behavior via GODEBUG, I don't see why this library should attempt to overrule their decision.

[iamemilio]

Ah ok, thats totally different. Agreed, we are making this a high priority bug. Thanks for clarifying the issue!

[nr-swilloughby]

The fix for this issue will be in the next release.