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

field/error: Handle panic in Error() #867

Merged
merged 4 commits into from Nov 24, 2020
Merged

field/error: Handle panic in Error() #867

merged 4 commits into from Nov 24, 2020

Conversation

@FMLS
Copy link
Contributor

@FMLS FMLS commented Sep 24, 2020

We shouldn't panic if the Error() method of an error panics.

type T struct{ msg string }

func (t *T) Error() string { return t.msg }

// The following panics.
var n *T = nil
log.Info("panic", zap.Error(n))
@CLAassistant
Copy link

@CLAassistant CLAassistant commented Sep 24, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

@codecov codecov bot commented Sep 24, 2020

Codecov Report

Merging #867 (421e615) into master (404189c) will decrease coverage by 0.14%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #867      +/-   ##
==========================================
- Coverage   98.36%   98.21%   -0.15%     
==========================================
  Files          43       43              
  Lines        2390     1910     -480     
==========================================
- Hits         2351     1876     -475     
+ Misses         32       27       -5     
  Partials        7        7              
Impacted Files Coverage Δ
zapcore/error.go 96.77% <100.00%> (+3.02%) ⬆️
zapcore/field.go 100.00% <100.00%> (ø)
internal/ztest/timeout.go 81.81% <0.00%> (-4.85%) ⬇️
zapcore/write_syncer.go 87.50% <0.00%> (-2.98%) ⬇️
internal/exit/exit.go 90.90% <0.00%> (-2.85%) ⬇️
zapcore/encoder.go 87.09% <0.00%> (-1.14%) ⬇️
zapcore/core.go 93.54% <0.00%> (-1.05%) ⬇️
zapcore/entry.go 93.82% <0.00%> (-0.80%) ⬇️
stacktrace.go 96.00% <0.00%> (-0.56%) ⬇️
global.go 96.66% <0.00%> (-0.48%) ⬇️
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 404189c...421e615. Read the comment docs.

@FMLS FMLS force-pushed the FMLS:master branch from fd79c57 to a5cead4 Sep 25, 2020
Copy link
Contributor

@prashantv prashantv left a comment

Thank you for the contribution @FMLS

Using reflect check for nil isn't always right, as it's possible to have a nil value that implements the error interface,
https://play.golang.org/p/5T8S35zdGsH

To handle nil in this case, please follow the same logic used by Stringer to detect a nil panic (which uses recover, similar to the standard library).

@FMLS FMLS force-pushed the FMLS:master branch from a5cead4 to d63f5ec Nov 4, 2020
@FMLS FMLS force-pushed the FMLS:master branch from d63f5ec to 1c0c739 Nov 4, 2020
zapcore/field.go Outdated Show resolved Hide resolved
zapcore/field.go Outdated Show resolved Hide resolved
zapcore/error.go Outdated Show resolved Hide resolved
zapcore/error.go Outdated Show resolved Hide resolved
zapcore/error.go Outdated Show resolved Hide resolved
error.go Outdated Show resolved Hide resolved
@abhinav abhinav changed the title BUGFIX: judge nil pointer stored in error interface field/error: Handle panic in Error() Nov 24, 2020
@abhinav abhinav requested a review from prashantv Nov 24, 2020
@abhinav abhinav merged commit 4b2b07c into uber-go:master Nov 24, 2020
4 checks passed
4 checks passed
@travis-ci
Travis CI - Pull Request Build Passed
Details
@codecov
codecov/patch 100.00% of diff hit (target 98.36%)
Details
@codecov
codecov/project 98.21% (target 95.00%)
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants