refactor(recovery): smart error comparison (#4142)

* refactor(recovery): rename var in CustomRecoveryWithWriter

* refactor(recovery): smart error comparison

* test(recovery): Directly reference the syscall error string
This commit is contained in:
OHZEKI Naoki 2026-01-17 17:40:43 +09:00 committed by GitHub
parent 9914178584
commit 3ab698dc51
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 22 additions and 28 deletions

View File

@ -12,12 +12,12 @@ import (
"fmt" "fmt"
"io" "io"
"log" "log"
"net"
"net/http" "net/http"
"net/http/httputil" "net/http/httputil"
"os" "os"
"runtime" "runtime"
"strings" "strings"
"syscall"
"time" "time"
"github.com/gin-gonic/gin/internal/bytesconv" "github.com/gin-gonic/gin/internal/bytesconv"
@ -57,40 +57,33 @@ func CustomRecoveryWithWriter(out io.Writer, handle RecoveryFunc) HandlerFunc {
} }
return func(c *Context) { return func(c *Context) {
defer func() { defer func() {
if err := recover(); err != nil { if rec := recover(); rec != nil {
// Check for a broken connection, as it is not really a // Check for a broken connection, as it is not really a
// condition that warrants a panic stack trace. // condition that warrants a panic stack trace.
var brokenPipe bool var isBrokenPipe bool
if ne, ok := err.(*net.OpError); ok { err, ok := rec.(error)
var se *os.SyscallError if ok {
if errors.As(ne, &se) { isBrokenPipe = errors.Is(err, syscall.EPIPE) ||
seStr := strings.ToLower(se.Error()) errors.Is(err, syscall.ECONNRESET) ||
if strings.Contains(seStr, "broken pipe") || errors.Is(err, http.ErrAbortHandler)
strings.Contains(seStr, "connection reset by peer") {
brokenPipe = true
}
}
}
if e, ok := err.(error); ok && errors.Is(e, http.ErrAbortHandler) {
brokenPipe = true
} }
if logger != nil { if logger != nil {
if brokenPipe { if isBrokenPipe {
logger.Printf("%s\n%s%s", err, secureRequestDump(c.Request), reset) logger.Printf("%s\n%s%s", rec, secureRequestDump(c.Request), reset)
} else if IsDebugging() { } else if IsDebugging() {
logger.Printf("[Recovery] %s panic recovered:\n%s\n%s\n%s%s", logger.Printf("[Recovery] %s panic recovered:\n%s\n%s\n%s%s",
timeFormat(time.Now()), secureRequestDump(c.Request), err, stack(stackSkip), reset) timeFormat(time.Now()), secureRequestDump(c.Request), rec, stack(stackSkip), reset)
} else { } else {
logger.Printf("[Recovery] %s panic recovered:\n%s\n%s%s", logger.Printf("[Recovery] %s panic recovered:\n%s\n%s%s",
timeFormat(time.Now()), err, stack(stackSkip), reset) timeFormat(time.Now()), rec, stack(stackSkip), reset)
} }
} }
if brokenPipe { if isBrokenPipe {
// If the connection is dead, we can't write a status to it. // If the connection is dead, we can't write a status to it.
c.Error(err.(error)) //nolint: errcheck c.Error(err) //nolint: errcheck
c.Abort() c.Abort()
} else { } else {
handle(c, err) handle(c, rec)
} }
} }
}() }()

View File

@ -98,13 +98,13 @@ func TestFunction(t *testing.T) {
func TestPanicWithBrokenPipe(t *testing.T) { func TestPanicWithBrokenPipe(t *testing.T) {
const expectCode = 204 const expectCode = 204
expectMsgs := map[syscall.Errno]string{ expectErrnos := []syscall.Errno{
syscall.EPIPE: "broken pipe", syscall.EPIPE,
syscall.ECONNRESET: "connection reset by peer", syscall.ECONNRESET,
} }
for errno, expectMsg := range expectMsgs { for _, errno := range expectErrnos {
t.Run(expectMsg, func(t *testing.T) { t.Run("Recovery from "+errno.Error(), func(t *testing.T) {
var buf strings.Builder var buf strings.Builder
router := New() router := New()
@ -122,7 +122,8 @@ func TestPanicWithBrokenPipe(t *testing.T) {
w := PerformRequest(router, http.MethodGet, "/recovery") w := PerformRequest(router, http.MethodGet, "/recovery")
// TEST // TEST
assert.Equal(t, expectCode, w.Code) assert.Equal(t, expectCode, w.Code)
assert.Contains(t, strings.ToLower(buf.String()), expectMsg) assert.Contains(t, strings.ToLower(buf.String()), errno.Error())
assert.NotContains(t, strings.ToLower(buf.String()), "[Recovery]")
}) })
} }
} }