stream: preserve falsy cancellation reasons
Project: nodejs/node | Language: JavaScript | Concepts: error-handling, api-design, testing-strategy | Difficulty: mid
Link: https://github.com/nodejs/node/pull/64705
The problem
When cancelling a stream with a "falsy" reason (like 0, '', false, or null), the cancellation reason was stored but not properly surfaced to consumers. Instead, consumers that had not yet started waiting would see the stream as having completed cleanly, rather than observing the supplied cancellation reason.
Why it's non-trivial
A naive fix might just check for "truthiness" of the error value, but this would still treat valid, intentional falsy reasons (like 0 or '') as "no error." This breaks the contract that cancellation reasons are always surfaced to consumers, regardless of their value. The code must distinguish between "no error" and "an error that happens to be falsy."
The approach
Use undefined as the "no error" sentinel
Previously, error state was tracked with null (which is falsy), and checks like if (self.#error) would skip over falsy reasons:
- #error = null;
+ #error;
By default, class fields in JavaScript are undefined unless initialized. This change means only undefined means "no error" — any other value (including null, 0, '', false) is treated as an error.
Explicitly check for undefined instead of truthiness
All error checks are changed from truthy checks to explicit undefined checks:
- if (self.#error) return PromiseReject(self.#error);
+ if (self.#error !== undefined) {
+ return PromiseReject(self.#error);
+ }
This ensures that any value (even falsy ones) will be surfaced as the cancellation reason.
Apply the same pattern everywhere errors are checked
This pattern is repeated throughout the broadcast, share, and sync share implementations:
- if (self.#error) {
+ if (self.#error !== undefined) {
state.detached = true;
self.#deleteConsumer(state);
return PromiseReject(self.#error);
And similarly for sync and async share implementations:
- if (self.#sourceError) {
+ if (self.#sourceError !== undefined) {
state.detached = true;
self.#consumers.delete(state);
throw self.#sourceError;
Update tests to cover falsy cancellation reasons
Tests are added to verify that all falsy values are correctly propagated as cancellation reasons:
+async function testShareCancelWithFalsyReason() {
+ for (const reason of [0, '', false, null]) {
+ const shared = share(from('data'));
+ const iterator = shared.pull()[Symbol.asyncIterator]();
+
+ shared.cancel(reason);
+
+ await assert.rejects(iterator.next(), (error) => error === reason);
+ }
+}
And similarly for broadcast and sync share:
+function testShareSyncCancelWithFalsyReason() {
+ for (const reason of [0, '', false, null]) {
+ const shared = shareSync(fromSync('data'));
+ const iterator = shared.pull()[Symbol.iterator]();
+
+ shared.cancel(reason);
+
+ assert.throws(() => iterator.next(), (error) => error === reason);
+ }
+}
From the review thread
No notable review discussion on this PR.
Takeaways
- Always use an explicit sentinel value (like
undefined) to represent "no error" or "unset" states, rather than relying on truthiness checks. - When designing APIs that accept arbitrary user-provided values (including falsy ones), ensure your logic distinguishes between "no value" and "a value that happens to be falsy."
- Update and expand tests to cover all edge cases, especially for values that are commonly mishandled due to language quirks (like
0,'',false, ornull).