Skip to content

Commit

Permalink
Don't set closed spans as active in withContext
Browse files Browse the repository at this point in the history
When a span is given to `ScopeManager.withContext` check if it is is
open. If it's not don't set it as active. Instead pass in the already
currently active span, or a NoopSpan if no span is active.

This prevents the usage of closed spans, that cannot be used to store
any data after they are closed.
  • Loading branch information
tombruijn committed Jun 3, 2022
1 parent 8a32a21 commit 4b74e2f
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 1 deletion.
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
bump: "patch"
type: "fix"
---

Don't return closed spans in `withSpan` helper. If a closed span was given to the `witSpan` helper it would temporarily overwrite the context with a closed span that cannot be modified. Instead it will return the current active span, if any. If no span was active it will return a `NoopSpan`.
28 changes: 28 additions & 0 deletions packages/nodejs/src/__tests__/scope.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { ScopeManager } from "../scope"
import { RootSpan, ChildSpan } from "../span"
import { Span } from "../interfaces/span"
import { EventEmitter } from "events"
import { NoopSpan } from "../noops/span"

describe("ScopeManager", () => {
let scopeManager: ScopeManager
Expand Down Expand Up @@ -183,6 +184,33 @@ describe("ScopeManager", () => {
})
})

it("when the given span is closed, it doesn't overwrite the current span", () => {
const span1 = new RootSpan()
scopeManager.setRoot(span1)

const span2 = new RootSpan()
span2.close()
scopeManager.setRoot(span2)

scopeManager.withContext(span2, spanInner => {
expect(scopeManager.root()).toBe(span1)
expect(scopeManager.active()).toBe(span1)
expect(spanInner).toBe(span1)
})
})

it("when the given span is closed, and there is no active span, it passes NoopSpan to the given function", () => {
const span1 = new RootSpan()
scopeManager.setRoot(span1)
span1.close()

scopeManager.withContext(span1, spanInner => {
expect(scopeManager.root()).toBeUndefined()
expect(scopeManager.active()).toBeUndefined()
expect(spanInner).toBeInstanceOf(NoopSpan)
})
})

it("restores the previous active span and root span", () => {
const outerRootSpan = new RootSpan()
const outerChildSpan = new ChildSpan(outerRootSpan)
Expand Down
7 changes: 6 additions & 1 deletion packages/nodejs/src/scope.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { Span } from "./interfaces/span"
import * as asyncHooks from "async_hooks"
import { EventEmitter } from "events"
import shimmer from "shimmer"
import { NoopSpan } from "./noops/span"

// A list of well-known EventEmitter methods that add event listeners.
const EVENT_EMITTER_ADD_METHODS: Array<keyof EventEmitter> = [
Expand Down Expand Up @@ -182,7 +183,11 @@ export class ScopeManager {
const oldScope = this.active()
const rootSpan = this.root()

this.#scopes.set(uid, span)
if (span.open) {
this.#scopes.set(uid, span)
} else {
span = oldScope || new NoopSpan()
}

try {
return fn(span)
Expand Down

0 comments on commit 4b74e2f

Please sign in to comment.