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

panics: allow usage of RecoveredPanic as a normal error #75

Merged
merged 3 commits into from
Jan 23, 2023

Conversation

bobheadxi
Copy link
Member

@bobheadxi bobheadxi commented Jan 21, 2023

Returning a concrete type *RecoveredPanic that implements error introduces an interesting gotcha - it looks like an error and behaves like an error, but cannot really be used as an error, because if returned as an error even if nil it's a typed error or something and shows up as not nil.

Example using the existing implementation:

func TestCatcherRecovered(t *testing.T) {
	var fn = func() error {
		var c Catcher
		c.Try(func() {})
		return c.Recovered() // ok, looks like an error
	}
	err := fn()
	assert.Nil(t, err)     // ok, surely no error
	assert.NoError(t, err) // fails!! err is nil but is not a nil error
	// Error: Received unexpected error:
	//   	  <nil>
}

Playground example: https://go.dev/play/p/C1DAdBWY9EZ , example in practice: https://github.com/sourcegraph/controller/actions/runs/3972347286/jobs/6810164688#step:7:141

To remedy this, we can either:

  • change (*Catcher).Recovered() *RecoveredPanic to (*Catcher).Recovered() error and ask that callers cast it into *RecoveredPanic for details a la os.ExitError
  • remove the error implementation and move it to another type that can be retrieved with (*RecoveredPanic).AsError()

This PR currently implements the latter - I might prefer the former, but it's a bigger change, and the distinction between a panic and an error returned is nice.

@codecov-commenter
Copy link

codecov-commenter commented Jan 21, 2023

Codecov Report

Merging #75 (d3de5bd) into main (e5ed75f) will decrease coverage by 0.26%.
The diff coverage is 90.00%.

❗ Current head d3de5bd differs from pull request most recent head 429a94b. Consider uploading reports for the commit 429a94b to get more accurate results

@@            Coverage Diff             @@
##             main      #75      +/-   ##
==========================================
- Coverage   99.46%   99.21%   -0.26%     
==========================================
  Files          11       11              
  Lines         374      380       +6     
==========================================
+ Hits          372      377       +5     
- Misses          2        3       +1     
Impacted Files Coverage Δ
panics/panics.go 97.14% <90.00%> (-2.86%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@camdencheek camdencheek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof, that's a rough one! Thanks for the fix. I agree that *RecoveredPanic should not implement error. I also agree with your proposed solution. IMO, panics should only rarely be turned into errors, so I think it's fine to have an explicit AsError() method rather than having Recovered() return an error directly.

Just a couple of small comments inline.

panics/panics.go Outdated Show resolved Hide resolved
panics/panics_test.go Outdated Show resolved Hide resolved
panics/panics_test.go Outdated Show resolved Hide resolved
panics/panics_test.go Outdated Show resolved Hide resolved
panics/panics_test.go Outdated Show resolved Hide resolved
Co-authored-by: Camden Cheek <camden@ccheek.com>
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 this pull request may close these issues.

3 participants