Don't panic when a given field value is nil #675
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #675 +/- ##
==========================================
+ Coverage 97.38% 97.39% +<.01%
==========================================
Files 40 40
Lines 2102 2108 +6
==========================================
+ Hits 2047 2053 +6
Misses 47 47
Partials 8 8
Continue to review full report at Codecov.
|
|
Thanks for the contribution @joa. Your solution works when dealing with functions that return interfaces, and explicitly return With your PR merged in, I wrote a simple test: func getURL() (*url.URL, error) {
return nil, nil
}
url, err := getURL()
if err != nil {
logger.Error("got error", zap.Error(err))
}
logger.Info("got url", zap.Stringer("url", url))This still causes a panic: See related questions: |
joa
force-pushed
the
master
branch
3 times, most recently
from
619ba17
to
a78f13d
Compare
|
Good catch. I expanded the test case and added the additional nil check. |
|
Unfortunately reflection comes with a performance penalty, and the only alternative is It's also not correct to assume that |
|
@prashantv The commit does not use reflection to test for the nil pointer in side the non-nil I understand the argument around a valid Of course both cases could be supported with |
|
I see from your comment in #495 (comment) that fmt is acutally doing this. Maybe that'd be an acceptable solution. |
|
With defer/recover Without defer/recover: |
|
Implemented recover within defer and expanded test cases as discussed (e.g. valid nil impl for Stringer). |
|
Thanks for adding the benchmarks @joa and some responses to your questions:
From the unsafe package documentation:
Even if the test covers it, a client may upgrade to a new Go version, and zap may stop working in that version, and will require a fix + library upgrades. I'd much rather avoid the concerns that crop up with unsafe.
Yep, the However, I do want to consider all the options, since this has come up multiple times. We could:
type safeStringer struct{
s fmt.Stringer
}
func (ss safeStringer) String() {
defer func() {
// recover and return "nil"
}()
return ss.s.String()
}
func SafeStringer(s fmt.Stringer) fmt.Stringer {
return safeStringer{s}
} |
|
Agree. There is another option. If you're just interested in the
Note that these benchmark numbers are to be taken with a grain of salt. |
|
Only doing If it's negligible, then it definitely makes sense to do by default. Otherwise, the "safe" field constructors could provide an alternative between safety and performance. |
|
@prashantv: Sorry for the late reply but here are the benchmarks Benchmark using
|
|
Thanks @joa. To better understand the performance impact, I wrote a benchmark which:
E.g., type customString string
func (cs customString) String() string {
return string(cs)
}
func BenchmarkStringer(b *testing.B) {
cfg := zap.NewProductionConfig()
cfg.OutputPaths = []string{"/dev/null"}
l, err := cfg.Build()
require.NoError(b, err, "failed to build logger")
cs := customString("encode")
b.Run("1 field", func(b *testing.B) {
for i := 0; i < b.N; i++ {
l.Info("test",
zap.Stringer("cs1", cs),
)
}
})
b.Run("4 fields", func(b *testing.B) {
for i := 0; i < b.N; i++ {
l.Info("test",
zap.Stringer("cs1", cs),
zap.Stringer("cs2", cs),
zap.Stringer("cs3", cs),
zap.Stringer("cs4", cs),
)
}
})
}To get a closer look at the impact to just the stringer performance, I wrote a benchmark which only logs stringer fields, I then compared the difference with and without this change on Go 1.12, Without this change (no With this change (with There's a very slight increase in the number of bytes allocated, but the actual number of allocations remains the same, and the performance is the same (the difference looks like noise). |
joa
force-pushed
the
master
branch
3 times, most recently
from
18a1593
to
38205dc
Compare
joa
force-pushed
the
master
branch
2 times, most recently
from
f204cf0
to
74cbeb7
Compare
|
Thank you for the review @prashantv and @abhinav. This MR became much more concise thanks to your feedback. |
This commit ensures any panic raised while encoding zap.Stringer is catched and reported as an error of the form kError for a field k. The reported panic is prefixed with "PANIC=" to distinguish these errors from other encoding errors. Closes uber-go#495 Closes uber-go#528
|
Might want to revisit in Go 1.13: https://go-review.googlesource.com/c/go/+/171758/ |
This commit ensures any panic raised while encoding zap.Stringer is caught and reported as an error as field kError for a field k. The reported panic is prefixed with "PANIC=" to distinguish these errors from other encoding errors. Closes uber-go#495
Ideally a logging framework doesn't cause a panic for code like this:
This is probably also fixing #495 and #528