diff --git a/v3/newrelic/internal_test.go b/v3/newrelic/internal_test.go index a1deb7878..acbc953de 100644 --- a/v3/newrelic/internal_test.go +++ b/v3/newrelic/internal_test.go @@ -753,6 +753,7 @@ func TestPanicInt(t *testing.T) { app.ExpectMetrics(t, backgroundErrorMetrics) } +/* superseded now by TestPanicNilRecovery. func TestPanicNil(t *testing.T) { app := testApp(nil, func(cfg *Config) { enableRecordPanics(cfg) @@ -769,6 +770,7 @@ func TestPanicNil(t *testing.T) { app.ExpectErrorEvents(t, []internal.WantEvent{}) app.ExpectMetrics(t, backgroundMetrics) } +*/ func TestResponseCodeError(t *testing.T) { app := testApp(nil, ConfigDistributedTracerEnabled(false), t) diff --git a/v3/newrelic/internal_txn.go b/v3/newrelic/internal_txn.go index 545083ca9..4a2523684 100644 --- a/v3/newrelic/internal_txn.go +++ b/v3/newrelic/internal_txn.go @@ -10,7 +10,6 @@ import ( "net/http" "net/url" "reflect" - "runtime" "runtime/debug" "sync" "time" @@ -446,16 +445,11 @@ func (thd *thread) End(recovered interface{}) error { txn.finished = true - // It used to be the case that panic(nil) would cause recover() to return nil, - // which we test for here. However, that is no longer the case, hence the extra - // check at this point to stop panic(nil) from propagating here. (as of Go 1.21) if recovered != nil { - if _, isNilPanic := recovered.(*runtime.PanicNilError); !isNilPanic { - e := txnErrorFromPanic(time.Now(), recovered) - e.Stack = getStackTrace() - thd.noticeErrorInternal(e, nil, false) - log.Println(string(debug.Stack())) - } + e := txnErrorFromPanic(time.Now(), recovered) + e.Stack = getStackTrace() + thd.noticeErrorInternal(e, nil, false) + log.Println(string(debug.Stack())) } txn.markEnd(time.Now(), thd.thread) @@ -547,13 +541,8 @@ func (thd *thread) End(recovered interface{}) error { } } - // Note that if a consumer uses `panic(nil)`, the panic will not - // propagate. Update: well, not anymore. Go now returns an actual - // non-nil value in this case. if recovered != nil { - if _, isNilPanic := recovered.(*runtime.PanicNilError); !isNilPanic { - panic(recovered) - } + panic(recovered) } return nil diff --git a/v3/newrelic/internal_txn_test.go b/v3/newrelic/internal_txn_test.go index bbe42c9d0..04d37bc28 100644 --- a/v3/newrelic/internal_txn_test.go +++ b/v3/newrelic/internal_txn_test.go @@ -7,6 +7,7 @@ import ( "errors" "net/http" "reflect" + "runtime" "testing" "time" @@ -981,3 +982,39 @@ func TestErrAttrsAddedWhenPanic(t *testing.T) { }, }) } + +func TestPanicNilRecovery(t *testing.T) { + cfgFnRecordPanics := func(cfg *Config) { + cfg.DistributedTracer.Enabled = true + cfg.ErrorCollector.RecordPanics = true + } + app := testApp(replyFn, cfgFnRecordPanics, t) + func() { + defer func() { + recovered := recover() + if recovered == nil { + t.Error("code did not panic as expected") + } else if _, isNil := recovered.(*runtime.PanicNilError); !isNil { + t.Errorf("code did not panic with nil as expected; got %v (type %T) instead", recovered, recovered) + } + }() + txn := app.StartTransaction("hello") + defer txn.End() + panic(nil) + }() + app.ExpectSpanEvents(t, []internal.WantEvent{ + { + AgentAttributes: map[string]interface{}{ + SpanAttributeErrorClass: "panic", + SpanAttributeErrorMessage: "panic called with nil argument", + }, + Intrinsics: map[string]interface{}{ + "category": internal.MatchAnything, + "timestamp": internal.MatchAnything, + "name": "OtherTransaction/Go/hello", + "transaction.name": "OtherTransaction/Go/hello", + "nr.entryPoint": true, + }, + }, + }) +}