Merge pull request #985 from nr-swilloughby/panic_nil

Remove special handling of panic(nil)

Author: iamemilio

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)

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

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,
+			},
+		},
+	})
+}