From 1650242cf54e49e8553f6db49a1c9cab1a101bd6 Mon Sep 17 00:00:00 2001 From: Gavin Shriver Date: Wed, 27 Mar 2024 12:43:38 -0700 Subject: [PATCH 1/6] gracefully handle elided frames --- internal/stack/stacks.go | 11 +++++++++++ internal/stack/stacks_test.go | 17 +++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/internal/stack/stacks.go b/internal/stack/stacks.go index 241a9b8..3c66e36 100644 --- a/internal/stack/stacks.go +++ b/internal/stack/stacks.go @@ -25,6 +25,7 @@ import ( "errors" "fmt" "io" + "regexp" "runtime" "strconv" "strings" @@ -32,6 +33,9 @@ import ( const _defaultBufferSize = 64 * 1024 // 64 KiB +// elidedFramesMatcher matches lines that indicate frames were elided; these cannot be parsed as functions. +var elidedFramesMatcher = regexp.MustCompile(`\.\.\.\s*\d+ frames elided\s*\.\.\.`) + // Stack represents a single Goroutine's stack. type Stack struct { id int @@ -158,6 +162,13 @@ func (p *stackParser) parseStack(line string) (Stack, error) { // Just skip it. continue } + if elidedFramesMatcher.MatchString(line) { + // e.g. ...23 frames elided... + // This indicates frames were elided from the stack trace, + // attempting to parse them via parseFuncName will fail resulting in a panic + // and a relatively useless output. Gracefully handle this. + continue + } funcName, creator, err := parseFuncName(line) if err != nil { diff --git a/internal/stack/stacks_test.go b/internal/stack/stacks_test.go index 156c2aa..3db593f 100644 --- a/internal/stack/stacks_test.go +++ b/internal/stack/stacks_test.go @@ -263,6 +263,23 @@ func TestParseStack(t *testing.T) { "example.com/foo/bar.baz", }, }, + { + name: "elided frames", + give: joinLines( + "goroutine 1 [running]:", + "example.com/foo/bar.baz()", + " example.com/foo/bar.go:123", + "... 3 frames elided ...", + "created by example.com/foo/bar.qux", + " example.com/foo/bar.go:456", + ), + id: 1, + state: "running", + firstFunc: "example.com/foo/bar.baz", + funcs: []string{ + "example.com/foo/bar.baz", + }, + }, } for _, tt := range tests { From a96f254bb27b1645ce45589aca65e648e281fa1a Mon Sep 17 00:00:00 2001 From: Gavin Shriver Date: Wed, 27 Mar 2024 12:52:36 -0700 Subject: [PATCH 2/6] make elided frames matcher more strict --- internal/stack/stacks.go | 2 +- internal/stack/stacks_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/stack/stacks.go b/internal/stack/stacks.go index 3c66e36..ff52805 100644 --- a/internal/stack/stacks.go +++ b/internal/stack/stacks.go @@ -34,7 +34,7 @@ import ( const _defaultBufferSize = 64 * 1024 // 64 KiB // elidedFramesMatcher matches lines that indicate frames were elided; these cannot be parsed as functions. -var elidedFramesMatcher = regexp.MustCompile(`\.\.\.\s*\d+ frames elided\s*\.\.\.`) +var elidedFramesMatcher = regexp.MustCompile(`\.\.\.\d+ frames elided\.\.\.`) // Stack represents a single Goroutine's stack. type Stack struct { diff --git a/internal/stack/stacks_test.go b/internal/stack/stacks_test.go index 3db593f..79d9200 100644 --- a/internal/stack/stacks_test.go +++ b/internal/stack/stacks_test.go @@ -269,7 +269,7 @@ func TestParseStack(t *testing.T) { "goroutine 1 [running]:", "example.com/foo/bar.baz()", " example.com/foo/bar.go:123", - "... 3 frames elided ...", + "...3 frames elided...", "created by example.com/foo/bar.qux", " example.com/foo/bar.go:456", ), From 810b5344862fe2b575617d3dac5c745e97f26ce3 Mon Sep 17 00:00:00 2001 From: Gavin Shriver Date: Fri, 12 Apr 2024 11:13:02 -0700 Subject: [PATCH 3/6] respond to PR feedback --- internal/stack/stacks.go | 6 +----- internal/stack/stacks_test.go | 12 ++++++++++-- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/internal/stack/stacks.go b/internal/stack/stacks.go index ff52805..1d31288 100644 --- a/internal/stack/stacks.go +++ b/internal/stack/stacks.go @@ -25,7 +25,6 @@ import ( "errors" "fmt" "io" - "regexp" "runtime" "strconv" "strings" @@ -33,9 +32,6 @@ import ( const _defaultBufferSize = 64 * 1024 // 64 KiB -// elidedFramesMatcher matches lines that indicate frames were elided; these cannot be parsed as functions. -var elidedFramesMatcher = regexp.MustCompile(`\.\.\.\d+ frames elided\.\.\.`) - // Stack represents a single Goroutine's stack. type Stack struct { id int @@ -162,7 +158,7 @@ func (p *stackParser) parseStack(line string) (Stack, error) { // Just skip it. continue } - if elidedFramesMatcher.MatchString(line) { + if strings.HasPrefix(line, "...") && strings.HasSuffix(line, " frames elided...") { // e.g. ...23 frames elided... // This indicates frames were elided from the stack trace, // attempting to parse them via parseFuncName will fail resulting in a panic diff --git a/internal/stack/stacks_test.go b/internal/stack/stacks_test.go index 79d9200..21d6c84 100644 --- a/internal/stack/stacks_test.go +++ b/internal/stack/stacks_test.go @@ -21,6 +21,7 @@ package stack import ( + "bytes" "os" "path/filepath" "runtime" @@ -136,8 +137,8 @@ func TestCurrentCreatedBy(t *testing.T) { func TestAllLargeStack(t *testing.T) { const ( - stackDepth = 100 - numGoroutines = 100 + stackDepth = 101 + numGoroutines = 101 ) var started sync.WaitGroup @@ -163,6 +164,13 @@ func TestAllLargeStack(t *testing.T) { t.Fatalf("Expected larger stack buffer") } + // Also test the stack parser here to ensure it handles elided frames, + // and that if the format elided frames take changes at any time we catch it. + // At the time of writing this test, with a stack depth of 101, we get 2 elided frames. + // "...2 frames elided...". + _, err := newStackParser(bytes.NewReader(buf)).Parse() + require.NoError(t, err) + // Start enough goroutines so we exceed the default buffer size. close(done) } From c192d3a0eeeaa2369be61bea7ed815c6825475ce Mon Sep 17 00:00:00 2001 From: Gavin Shriver Date: Fri, 12 Apr 2024 11:17:02 -0700 Subject: [PATCH 4/6] comment fix --- internal/stack/stacks_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/stack/stacks_test.go b/internal/stack/stacks_test.go index 21d6c84..aaac399 100644 --- a/internal/stack/stacks_test.go +++ b/internal/stack/stacks_test.go @@ -165,8 +165,8 @@ func TestAllLargeStack(t *testing.T) { } // Also test the stack parser here to ensure it handles elided frames, - // and that if the format elided frames take changes at any time we catch it. - // At the time of writing this test, with a stack depth of 101, we get 2 elided frames. + // and that if the format elided frames changes at any time we catch it. + // At the time of writing this test, with a stack depth of 101, we get 2 elided frames: // "...2 frames elided...". _, err := newStackParser(bytes.NewReader(buf)).Parse() require.NoError(t, err) From 71a9df0b630cd6d4bf08c959f522ef42bffe5961 Mon Sep 17 00:00:00 2001 From: Gavin Shriver Date: Fri, 12 Apr 2024 11:20:56 -0700 Subject: [PATCH 5/6] another comment tweak for clarity --- internal/stack/stacks_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/stack/stacks_test.go b/internal/stack/stacks_test.go index aaac399..9879454 100644 --- a/internal/stack/stacks_test.go +++ b/internal/stack/stacks_test.go @@ -164,8 +164,8 @@ func TestAllLargeStack(t *testing.T) { t.Fatalf("Expected larger stack buffer") } - // Also test the stack parser here to ensure it handles elided frames, - // and that if the format elided frames changes at any time we catch it. + // Also test the stack parser here to ensure it handles elided frames gracefully. + // We want to check this here, so that if the format of the "elided frames" message changes, we catch it. // At the time of writing this test, with a stack depth of 101, we get 2 elided frames: // "...2 frames elided...". _, err := newStackParser(bytes.NewReader(buf)).Parse() From effe6af0a8c4f65d037811dc8852ef705f4c2f09 Mon Sep 17 00:00:00 2001 From: Prashant Varanasi Date: Fri, 12 Apr 2024 20:07:34 -0700 Subject: [PATCH 6/6] Verify large stack has elided frames --- internal/stack/stacks_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/stack/stacks_test.go b/internal/stack/stacks_test.go index 9879454..b1a21d8 100644 --- a/internal/stack/stacks_test.go +++ b/internal/stack/stacks_test.go @@ -168,8 +168,10 @@ func TestAllLargeStack(t *testing.T) { // We want to check this here, so that if the format of the "elided frames" message changes, we catch it. // At the time of writing this test, with a stack depth of 101, we get 2 elided frames: // "...2 frames elided...". - _, err := newStackParser(bytes.NewReader(buf)).Parse() + assert.Contains(t, string(buf), "frames elided...") + stacks, err := newStackParser(bytes.NewReader(buf)).Parse() require.NoError(t, err) + assert.Greater(t, len(stacks), numGoroutines, "expect more parsed stacks than goroutines") // Start enough goroutines so we exceed the default buffer size. close(done)