Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 46 additions & 24 deletions lib/diagnostics_channel.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,8 @@ class RunStoresScope {
#stack;

constructor(activeChannel, data) {
// eslint-disable-next-line no-restricted-globals
using stack = new DisposableStack();
// TODO: use native DisposableStack once supported in core
const stack = [];
Comment thread
Renegade334 marked this conversation as resolved.

// Enter stores using withScope
if (activeChannel._stores) {
Expand All @@ -109,19 +109,27 @@ class RunStoresScope {
}
}

stack.use(store.withScope(newContext));
ArrayPrototypePush(stack, store.withScope(newContext));
}
}

// Publish data
activeChannel.publish(data);

// Transfer ownership of the stack
this.#stack = stack.move();
// TODO: Restore stack.move() when DisposableStack is restored in core to avoid broken behavior.
this.#stack = stack;
}

[SymbolDispose]() {
this.#stack[SymbolDispose]();
if (this.#stack !== undefined) {
const stack = this.#stack;
this.#stack = undefined;

for (let i = stack.length - 1; i >= 0; i--) {
stack[i][SymbolDispose]();
}
}
}
}

Expand Down Expand Up @@ -197,9 +205,12 @@ class ActiveChannel {
}

runStores(data, fn, thisArg, ...args) {
// eslint-disable-next-line no-unused-vars
using scope = this.withStoreScope(data);
return ReflectApply(fn, thisArg, args);
const scope = this.withStoreScope(data);
try {
return ReflectApply(fn, thisArg, args);
} finally {
scope[SymbolDispose]();
}
}
}

Expand Down Expand Up @@ -396,9 +407,12 @@ class BoundedChannel {

run(context, fn, thisArg, ...args) {
context ??= {};
// eslint-disable-next-line no-unused-vars
using scope = this.withScope(context);
return ReflectApply(fn, thisArg, args);
const scope = this.withScope(context);
try {
return ReflectApply(fn, thisArg, args);
} finally {
scope[SymbolDispose]();
}
}
}

Expand Down Expand Up @@ -520,9 +534,8 @@ class TracingChannel {
}

const { error } = this;
const scope = this.#callWindow.withScope(context);

// eslint-disable-next-line no-unused-vars
using scope = this.#callWindow.withScope(context);
try {
const result = ReflectApply(fn, thisArg, args);
context.result = result;
Expand All @@ -531,6 +544,8 @@ class TracingChannel {
context.error = err;
error.publish(context);
throw err;
} finally {
scope[SymbolDispose]();
}
}

Expand All @@ -550,9 +565,9 @@ class TracingChannel {
context.error = err;
error.publish(context);
// Use continuation window for asyncStart/asyncEnd
// eslint-disable-next-line no-unused-vars
using scope = continuationWindow.withScope(context);
const scope = continuationWindow.withScope(context);
// TODO: Is there a way to have asyncEnd _after_ the continuation?
scope[SymbolDispose]();
}

function onRejectWithRethrow(err) {
Expand All @@ -563,14 +578,14 @@ class TracingChannel {
function onResolve(result) {
context.result = result;
// Use continuation window for asyncStart/asyncEnd
// eslint-disable-next-line no-unused-vars
using scope = continuationWindow.withScope(context);
const scope = continuationWindow.withScope(context);
// TODO: Is there a way to have asyncEnd _after_ the continuation?
scope[SymbolDispose]();
return result;
}

// eslint-disable-next-line no-unused-vars
using scope = this.#callWindow.withScope(context);
const scope = this.#callWindow.withScope(context);

try {
const result = ReflectApply(fn, thisArg, args);
// If the return value is not a thenable, return it directly with a warning.
Expand All @@ -595,6 +610,8 @@ class TracingChannel {
context.error = err;
error.publish(context);
throw err;
} finally {
scope[SymbolDispose]();
}
}

Expand All @@ -615,23 +632,28 @@ class TracingChannel {
}

// Use continuation window for asyncStart/asyncEnd around callback
// eslint-disable-next-line no-unused-vars
using scope = continuationWindow.withScope(context);
return ReflectApply(callback, this, arguments);
const scope = continuationWindow.withScope(context);
try {
return ReflectApply(callback, this, arguments);
} finally {
scope[SymbolDispose]();
}
}

const callback = ArrayPrototypeAt(args, position);
validateFunction(callback, 'callback');
ArrayPrototypeSplice(args, position, 1, wrappedCallback);

// eslint-disable-next-line no-unused-vars
using scope = this.#callWindow.withScope(context);
const scope = this.#callWindow.withScope(context);

try {
return ReflectApply(fn, thisArg, args);
} catch (err) {
context.error = err;
error.publish(context);
throw err;
} finally {
scope[SymbolDispose]();
}
}
}
Expand Down
11 changes: 11 additions & 0 deletions test/parallel/test-diagnostics-channel-no-erm.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Flags: --no-js-explicit-resource-management

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not add a test for an unsupported V8 flag and instead make this change as part of the lint rule (as a separate commit, but in the same PR)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback, everyone.

To make sure we're aligned on the policy around V8 flags and Stage 4 syntax before I update the branch:

@aduh95 — I am happy to drop test/parallel/test-diagnostics-channel-no-erm.js so we aren't adding tests for an unsupported V8 flag, and instead enforce this via a lint rule in a separate commit within this PR.

@anonrig, @Renegade334 — Does pivoting from an explicit V8 flag test to a lint-rule enforcement (as suggested by @aduh95) resolve the remaining concerns here?

If everyone is on board with that approach, I'll push those changes today. Otherwise, if the consensus is that Stage 4 ERM should remain unrestricted in core regardless of --no-js-explicit-resource-management, let me know and I can close this out.

'use strict';

require('../common');
const { tracingChannel } = require('diagnostics_channel');

// This test ensures that using diagnostics_channel does not cause a segfault
// when explicit resource management is disabled in V8.
// Refs: https://github.com/nodejs/node/issues/64230

tracingChannel('foo').traceSync(() => {});
Loading