Skip to content

Commit

Permalink
Don't restore active span if closed in withContext
Browse files Browse the repository at this point in the history
When a span gets closed for whatever reason before getting restored in a
withContext callback, do not restore it. We do not restore closed spans
as active spans.

This is difficult to test, because the `active()` function also does
this check, but removing that check this test will still pass.
  • Loading branch information
tombruijn committed Jun 3, 2022
1 parent 7a7bc9b commit b7382d7
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 4 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
bump: "patch"
type: "change"
---

Do not restore closed spans from within the `withSpan` helper. If a previously active span gets closed while `withSpan` has another span as currently active, do not restore the closed span when the callback has finished.
20 changes: 20 additions & 0 deletions packages/nodejs/src/__tests__/scope.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,26 @@ describe("ScopeManager", () => {
expect(scopeManager.active()).toBe(outerRootSpan)
expect(scopeManager.root()).toBe(innerRootSpan)
})

it("does not restore the active span if it is closed", () => {
const rootSpan = new RootSpan()
const activeSpan1 = new ChildSpan(rootSpan)
const activeSpan2 = new ChildSpan(rootSpan)

scopeManager.setRoot(rootSpan)
expect(scopeManager.active()).toBe(rootSpan)

scopeManager.withContext(activeSpan1, () => {
expect(scopeManager.active()).toBe(activeSpan1)
scopeManager.withContext(activeSpan2, () => {
activeSpan1.close()
expect(scopeManager.active()).toBe(activeSpan2)
})
expect(scopeManager.active()).toBeUndefined()
})

expect(scopeManager.active()).toBe(rootSpan)
})
})

describe(".bindContext()", () => {
Expand Down
9 changes: 5 additions & 4 deletions packages/nodejs/src/scope.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,10 +194,11 @@ export class ScopeManager {
this.root()?.setError(err)
throw err
} finally {
// revert to the previous span
if (oldScope === undefined) {
this.#scopes.delete(uid)
} else {
// Unset the current active span so it doesn't leak outside this context
// in case there was no previous active span or it's no longer open.
this.#scopes.delete(uid)
if (oldScope && oldScope.open) {
// Revert the current active span
this.#scopes.set(uid, oldScope)
}
}
Expand Down

0 comments on commit b7382d7

Please sign in to comment.