Skip to content
This repository has been archived by the owner on Mar 6, 2023. It is now read-only.

Commit

Permalink
Improve the "use-after-free" assertion
Browse files Browse the repository at this point in the history
Let it print out the "previous" contents of the Span when it triggers
a use-after-free assertion. This is valuable in tracking down the past
of that Span, and thus the original call to Finish().
  • Loading branch information
tbg committed Sep 28, 2016
1 parent 770ba1a commit 278bf6f
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 7 deletions.
9 changes: 8 additions & 1 deletion concurrency_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package basictracer

import (
"strings"
"sync"
"testing"

Expand Down Expand Up @@ -40,19 +41,25 @@ func TestDebugAssertUseAfterFinish(t *testing.T) {
opts.Recorder = NewInMemoryRecorder()
opts.DebugAssertUseAfterFinish = true
tracer := NewWithOptions(opts)
const msg = "I shall be finished"
for _, double := range []bool{false, true} {
sp := tracer.StartSpan(op)
sp.Log(opentracing.LogData{Event: msg})
if double {
sp.Finish()
}
var panicked bool
func() {
defer func() {
r := recover()
_, panicked = r.(*errAssertionFailed)
var assertionErr error
assertionErr, panicked = r.(*errAssertionFailed)
if !panicked && r != nil {
panic(r)
}
if panicked && !strings.Contains(assertionErr.Error(), msg) {
t.Fatalf("debug output did not contain log message '%s': %+v", msg, assertionErr)
}
spImpl := sp.(*spanImpl)
// The panic should leave the Mutex unlocked.
spImpl.Mutex.Lock()
Expand Down
15 changes: 10 additions & 5 deletions debug.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,13 @@ import (
const debugGoroutineIDTag = "_initial_goroutine"

type errAssertionFailed struct {
msg string
span *spanImpl
msg string
}

// Error implements the error interface.
func (err *errAssertionFailed) Error() string {
return fmt.Sprintf("%s:\n%+v", err.msg, err.span)
}

func (s *spanImpl) Lock() {
Expand All @@ -22,9 +28,7 @@ func (s *spanImpl) Lock() {
func (s *spanImpl) maybeAssertSanityLocked() {
if s.tracer == nil {
s.Mutex.Unlock()
panic(&errAssertionFailed{
msg: fmt.Sprintf("span used after Finish()"),
})
panic(&errAssertionFailed{span: s, msg: "span used after call to Finish()"})
}
if s.tracer.options.DebugAssertSingleGoroutine {
startID := curGoroutineID()
Expand All @@ -37,7 +41,8 @@ func (s *spanImpl) maybeAssertSanityLocked() {
if startID != curID {
s.Mutex.Unlock()
panic(&errAssertionFailed{
msg: fmt.Sprintf("span started on goroutine %d, but now running on %d", startID, curID),
span: s,
msg: fmt.Sprintf("span started on goroutine %d, but now running on %d", startID, curID),
})
}
}
Expand Down
4 changes: 3 additions & 1 deletion span.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,9 @@ func (s *spanImpl) FinishWithOptions(opts opentracing.FinishOptions) {
if s.tracer.options.DebugAssertUseAfterFinish {
// This makes it much more likely to catch a panic on any subsequent
// operation since s.tracer is accessed on every call to `Lock`.
s.reset()
// We don't call `reset()` here to preserve the logs in the Span
// which are printed when the assertion triggers.
s.tracer = nil
}

if poolEnabled {
Expand Down

0 comments on commit 278bf6f

Please sign in to comment.