From f46c0cd341889dffeb126774c6b6f7e87f0575c3 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Sun, 22 Oct 2023 11:12:03 -0700 Subject: [PATCH] stack: "created by" is not part of the stack The function tha created a goroutine should not be considered part of its stack. However, we can use that entry to mark the end of a stack trace. --- internal/stack/stacks.go | 52 +++++++++++++++++++++++++++-------- internal/stack/stacks_test.go | 38 ++++++++++++++----------- 2 files changed, 62 insertions(+), 28 deletions(-) diff --git a/internal/stack/stacks.go b/internal/stack/stacks.go index 4c8af9b..241a9b8 100644 --- a/internal/stack/stacks.go +++ b/internal/stack/stacks.go @@ -159,13 +159,21 @@ func (p *stackParser) parseStack(line string) (Stack, error) { continue } - funcName, err := parseFuncName(line) + funcName, creator, err := parseFuncName(line) if err != nil { return Stack{}, fmt.Errorf("parse function: %w", err) } - funcs[funcName] = struct{}{} - if firstFunction == "" { - firstFunction = funcName + if !creator { + // A function is part of a goroutine's stack + // only if it's not a "created by" function. + // + // The creator function is part of a different stack. + // We don't care about it right now. + funcs[funcName] = struct{}{} + if firstFunction == "" { + firstFunction = funcName + } + } // The function name followed by a line in the form: @@ -180,12 +188,30 @@ func (p *stackParser) parseStack(line string) (Stack, error) { if len(bs) > 0 && bs[0] == '\t' { fullStack.Write(bs) fullStack.WriteByte('\n') - continue + } else { + // Put it back and let the next iteration handle it + // if it doesn't start with a tab. + p.scan.Unscan() } + } - // Put it back and let the next iteration handle it - // if it doesn't start with a tab. - p.scan.Unscan() + if creator { + // The "created by" line is the last line of the stack. + // We can stop parsing now. + // + // Note that if tracebackancestors=N is set, + // there may be more a traceback of the creator function + // following the "created by" line, + // but it should not be considered part of this stack. + // e.g., + // + // created by testing.(*T).Run in goroutine 1 + // /usr/lib/go/src/testing/testing.go:1648 +0x3ad + // [originating from goroutine 1]: + // testing.(*T).Run(...) + // /usr/lib/go/src/testing/testing.go:1649 +0x3ad + // + break } } @@ -224,8 +250,9 @@ func getStackBuffer(all bool) []byte { // example.com/path/to/package.(*typeName).funcName(args...) // created by example.com/path/to/package.funcName // created by example.com/path/to/package.funcName in goroutine [...] -func parseFuncName(line string) (string, error) { - var name string +// +// Also reports whether the line was a "created by" line. +func parseFuncName(line string) (name string, creator bool, err error) { if after, ok := strings.CutPrefix(line, "created by "); ok { // The function name is the part after "created by " // and before " in goroutine [...]". @@ -234,16 +261,17 @@ func parseFuncName(line string) (string, error) { after = after[:idx] } name = after + creator = true } else if idx := strings.LastIndexByte(line, '('); idx >= 0 { // The function name is the part before the last '('. name = line[:idx] } if name == "" { - return "", fmt.Errorf("no function found: %q", line) + return "", false, fmt.Errorf("no function found: %q", line) } - return name, nil + return name, creator, nil } // parseGoStackHeader parses a stack header that looks like: diff --git a/internal/stack/stacks_test.go b/internal/stack/stacks_test.go index 9558ec6..4f72049 100644 --- a/internal/stack/stacks_test.go +++ b/internal/stack/stacks_test.go @@ -102,9 +102,6 @@ func TestCurrent(t *testing.T) { assert.Contains(t, got.String(), "in state") assert.Contains(t, got.String(), "on top of the stack") - assert.True(t, got.HasFunction("testing.(*T).Run"), - "missing in stack: %v\n%s", "testing.(*T).Run", all) - assert.Contains(t, all, "stack/stacks_test.go", "file name missing in stack:\n%s", all) @@ -124,10 +121,15 @@ func TestCurrentCreatedBy(t *testing.T) { }() <-done - // This test function will be at the bottom of the stack - // as the function that created the goroutine. + // The test function created the goroutine + // so it won't be part of the stack. + assert.False(t, stack.HasFunction("go.uber.org/goleak/internal/stack.TestCurrentCreatedBy"), + "TestCurrentCreatedBy should not be in stack:\n%s", stack.Full()) + + // However, the nested function should be. assert.True(t, - stack.HasFunction("go.uber.org/goleak/internal/stack.TestCurrentCreatedBy")) + stack.HasFunction("go.uber.org/goleak/internal/stack.TestCurrentCreatedBy.func1"), + "TestCurrentCreatedBy.func1 is not in stack:\n%s", stack.Full()) } func TestAllLargeStack(t *testing.T) { @@ -165,9 +167,10 @@ func TestAllLargeStack(t *testing.T) { func TestParseFuncName(t *testing.T) { tests := []struct { - name string - give string - want string + name string + give string + want string + creator bool }{ { name: "function", @@ -180,22 +183,25 @@ func TestParseFuncName(t *testing.T) { want: "example.com/foo/bar.(*baz).qux", }, { - name: "created by", // Go 1.20 - give: "created by example.com/foo/bar.baz", - want: "example.com/foo/bar.baz", + name: "created by", // Go 1.20 + give: "created by example.com/foo/bar.baz", + want: "example.com/foo/bar.baz", + creator: true, }, { - name: "created by/in goroutine", // Go 1.21 - give: "created by example.com/foo/bar.baz in goroutine 123", - want: "example.com/foo/bar.baz", + name: "created by/in goroutine", // Go 1.21 + give: "created by example.com/foo/bar.baz in goroutine 123", + want: "example.com/foo/bar.baz", + creator: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := parseFuncName(tt.give) + got, creator, err := parseFuncName(tt.give) require.NoError(t, err) assert.Equal(t, tt.want, got) + assert.Equal(t, tt.creator, creator) }) } }