Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs, stream: add initial Symbol.dispose and Symbol.asyncDispose support #48518

Merged
merged 10 commits into from
Jun 25, 2023
8 changes: 8 additions & 0 deletions doc/api/fs.md
Original file line number Diff line number Diff line change
Expand Up @@ -817,6 +817,14 @@ On Linux, positional writes don't work when the file is opened in append mode.
The kernel ignores the position argument and always appends the data to
the end of the file.
#### `filehandle[Symbol.asyncDispose]()`
MoLow marked this conversation as resolved.
Show resolved Hide resolved
<!-- YAML
added: REPLACEME
-->
An alias for `filehandle.close()`.
### `fsPromises.access(path[, mode])`
<!-- YAML
Expand Down
8 changes: 8 additions & 0 deletions doc/api/stream.md
Original file line number Diff line number Diff line change
Expand Up @@ -1904,6 +1904,14 @@ option. In the code example above, data will be in a single chunk if the file
has less then 64 KiB of data because no `highWaterMark` option is provided to
[`fs.createReadStream()`][].

##### `readable[Symbol.asyncDispose]()`
MoLow marked this conversation as resolved.
Show resolved Hide resolved

<!-- YAML
added: REPLACEME
-->

An alias for [`readable.destroy()`][readable-destroy] with an `AbortError`.
benjamingr marked this conversation as resolved.
Show resolved Hide resolved
benjamingr marked this conversation as resolved.
Show resolved Hide resolved

##### `readable.compose(stream[, options])`

<!-- YAML
Expand Down
5 changes: 5 additions & 0 deletions lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const {
SafeArrayIterator,
SafePromisePrototypeFinally,
Symbol,
SymbolAsyncDispose,
Uint8Array,
FunctionPrototypeBind,
} = primordials;
Expand Down Expand Up @@ -246,6 +247,10 @@ class FileHandle extends EventEmitterMixin(JSTransferable) {
return this[kClosePromise];
};

async [SymbolAsyncDispose]() {
return this.close();
}

/**
* @typedef {import('../webstreams/readablestream').ReadableStream
* } ReadableStream
Expand Down
5 changes: 5 additions & 0 deletions lib/internal/per_context/primordials.js
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,11 @@ function copyPrototype(src, dest, prefix) {
copyPrototype(original.prototype, primordials, `${name}Prototype`);
});

// Define Symbol.Disposed and Symbol.AsyncDispose
// Until these are defined by the environment.
MoLow marked this conversation as resolved.
Show resolved Hide resolved
primordials.SymbolDispose ??= primordials.SymbolFor('nodejs.dispose');
MoLow marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this mean Symbol.keyFor of these symbols won't properly return undefined?

primordials.SymbolAsyncDispose ??= primordials.SymbolFor('nodejs.asyncDispose');

// Create copies of intrinsic objects that require a valid `this` to call
// static methods.
// Refs: https://www.ecma-international.org/ecma-262/#sec-promise.all
Expand Down
10 changes: 10 additions & 0 deletions lib/internal/process/pre_execution.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ const {
ObjectGetOwnPropertyDescriptor,
SafeMap,
StringPrototypeStartsWith,
Symbol,
SymbolDispose,
SymbolAsyncDispose,
globalThis,
} = primordials;

Expand Down Expand Up @@ -82,6 +85,8 @@ function prepareExecution(options) {

require('internal/dns/utils').initializeDns();

setupSymbolDisposePolyfill();

if (isMainThread) {
assert(internalBinding('worker').isMainThread);
// Worker threads will get the manifest in the message handler.
Expand Down Expand Up @@ -119,6 +124,11 @@ function prepareExecution(options) {
}
}

function setupSymbolDisposePolyfill() {
MoLow marked this conversation as resolved.
Show resolved Hide resolved
Symbol.dispose ??= SymbolDispose;
Symbol.asyncDispose ??= SymbolAsyncDispose;
Copy link
Member

Choose a reason for hiding this comment

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

Would this be a good thing to polyfill them? Wouldn't users assume certain behaviors from the runtime because they exists?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think there is a way for typescript or bable to recognize these symbols without them being global.

Copy link
Member

Choose a reason for hiding this comment

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

@mcollina this is confirmed (with the TS team) to play nice. Users would be able to use them with the polyfill but not the syntactic sugar until v8 ships

Copy link

Choose a reason for hiding this comment

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

That breaks core-js.

Copy link

Choose a reason for hiding this comment

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

}

function setupUserModules(isLoaderWorker = false) {
initializeCJSLoader();
initializeESMLoader(isLoaderWorker);
Expand Down
11 changes: 11 additions & 0 deletions lib/internal/streams/readable.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const {
ObjectSetPrototypeOf,
Promise,
SafeSet,
SymbolAsyncDispose,
SymbolAsyncIterator,
Symbol,
} = primordials;
Expand Down Expand Up @@ -67,6 +68,7 @@ const {
ERR_STREAM_UNSHIFT_AFTER_END_EVENT,
ERR_UNKNOWN_ENCODING,
},
AbortError,
} = require('internal/errors');
const { validateObject } = require('internal/validators');

Expand Down Expand Up @@ -234,6 +236,15 @@ Readable.prototype[EE.captureRejectionSymbol] = function(err) {
this.destroy(err);
};

Readable.prototype[SymbolAsyncDispose] = function() {
let error;
if (!this.destroyed) {
error = new AbortError();
benjamingr marked this conversation as resolved.
Show resolved Hide resolved
this.destroy(error);
}
return new Promise((resolve, reject) => eos(this, (err) => (err !== error ? reject(err) : resolve(null))));
benjamingr marked this conversation as resolved.
Show resolved Hide resolved
};

// Manually shove something into the read() buffer.
// This returns true if the highWaterMark has not been hit yet,
// similar to how Writable.write() returns true if you should
Expand Down
12 changes: 12 additions & 0 deletions test/parallel/test-fs-promises-file-handle-dispose.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
'use strict';

const common = require('../common');
const { promises: fs } = require('fs');

async function doOpen() {
const fh = await fs.open(__filename);
fh.on('close', common.mustCall());
await fh[Symbol.asyncDispose]();
}

doOpen().then(common.mustCall());
23 changes: 23 additions & 0 deletions test/parallel/test-stream-readable-dispose.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';

const common = require('../common');
const { Readable } = require('stream');
const assert = require('assert');

{
const read = new Readable({
read() {}
});
read.resume();

read.on('end', common.mustNotCall('no end event'));
read.on('close', common.mustCall());
read.on('error', common.mustCall((err) => {
assert.strictEqual(err.name, 'AbortError');
}));

read[Symbol.asyncDispose]().then(common.mustCall(() => {
assert.strictEqual(read.errored.name, 'AbortError');
assert.strictEqual(read.destroyed, true);
}));
}
2 changes: 2 additions & 0 deletions typings/primordials.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,8 @@ declare namespace primordials {
export const SymbolFor: typeof Symbol.for
export const SymbolKeyFor: typeof Symbol.keyFor
export const SymbolAsyncIterator: typeof Symbol.asyncIterator
export const SymbolDispose: typeof Symbol
export const SymbolAsyncDispose: typeof Symbol
MoLow marked this conversation as resolved.
Show resolved Hide resolved
export const SymbolHasInstance: typeof Symbol.hasInstance
export const SymbolIsConcatSpreadable: typeof Symbol.isConcatSpreadable
export const SymbolIterator: typeof Symbol.iterator
Expand Down