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

feat: disposable Deno resources #20845

Merged
merged 11 commits into from
Nov 1, 2023
Merged
Show file tree
Hide file tree
Changes from 6 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
27 changes: 27 additions & 0 deletions cli/tests/unit/files_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -824,3 +824,30 @@ Deno.test(
assertEquals(res, "hello \uFFFD");
},
);

Deno.test(
{ permissions: { read: true } },
async function fsFileExplicitResourceManagement() {
let file2: Deno.FsFile;

{
using file = await Deno.open("cli/tests/testdata/assets/hello.txt");
file2 = file;

const stat = file.statSync();
assert(stat.isFile);
}

assertThrows(() => file2.statSync(), Deno.errors.BadResource);
},
);

Deno.test(
{ permissions: { read: true } },
async function fsFileExplicitResourceManagementManualClose() {
using file = await Deno.open("cli/tests/testdata/assets/hello.txt");
file.close();
assertThrows(() => file.statSync(), Deno.errors.BadResource); // definitely closed
// calling [Symbol.dispose] after manual close is a no-op
},
);
31 changes: 31 additions & 0 deletions cli/tests/unit/net_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1242,3 +1242,34 @@ Deno.test({
const listener = Deno.listen({ hostname: "localhost", port: "0" });
listener.close();
});

Deno.test(
{ permissions: { net: true } },
async function listenerExplicitResourceManagement() {
let done: Promise<Deno.errors.BadResource>;

{
using listener = Deno.listen({ port: listenPort });

done = assertRejects(
() => listener.accept(),
Deno.errors.BadResource,
);
}

await done;
},
);

Deno.test(
{ permissions: { net: true } },
async function listenerExplicitResourceManagementManualClose() {
using listener = Deno.listen({ port: listenPort });
listener.close();
await assertRejects( // definitely closed
() => listener.accept(),
Deno.errors.BadResource,
);
// calling [Symbol.dispose] after manual close is a no-op
},
);
6 changes: 5 additions & 1 deletion cli/tsc/99_main_compiler.js
Original file line number Diff line number Diff line change
Expand Up @@ -1138,6 +1138,7 @@ delete Object.prototype.__proto__;
`${ASSETS_URL_PREFIX}${specifier}`,
ts.ScriptTarget.ESNext,
),
`failed to load '${ASSETS_URL_PREFIX}${specifier}'`,
);
}
// this helps ensure as much as possible is in memory that is re-usable
Expand All @@ -1148,7 +1149,10 @@ delete Object.prototype.__proto__;
options: SNAPSHOT_COMPILE_OPTIONS,
host,
});
assert(ts.getPreEmitDiagnostics(TS_SNAPSHOT_PROGRAM).length === 0);
assert(
ts.getPreEmitDiagnostics(TS_SNAPSHOT_PROGRAM).length === 0,
"lib.d.ts files have errors",
);

// remove this now that we don't need it anymore for warming up tsc
sourceFileCache.delete(buildSpecifier);
Expand Down
16 changes: 11 additions & 5 deletions cli/tsc/dts/lib.deno.ns.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

/// <reference no-default-lib="true" />
/// <reference lib="esnext" />
/// <reference lib="esnext.disposable" />
/// <reference lib="deno.net" />

/** Deno provides extra properties on `import.meta`. These are included here
Expand Down Expand Up @@ -2177,7 +2178,8 @@ declare namespace Deno {
WriterSync,
Seeker,
SeekerSync,
Closer {
Closer,
Disposable {
/** The resource ID associated with the file instance. The resource ID
* should be considered an opaque reference to resource. */
readonly rid: number;
Expand Down Expand Up @@ -2451,6 +2453,8 @@ declare namespace Deno {
* ```
*/
close(): void;

[Symbol.dispose](): void;
}

/**
Expand Down Expand Up @@ -3831,7 +3835,7 @@ declare namespace Deno {
*
* @category File System
*/
export interface FsWatcher extends AsyncIterable<FsEvent> {
export interface FsWatcher extends AsyncIterable<FsEvent>, Disposable {
/** The resource id. */
readonly rid: number;
/** Stops watching the file system and closes the watcher resource. */
Expand Down Expand Up @@ -4284,7 +4288,7 @@ declare namespace Deno {
*
* @category Sub Process
*/
export class ChildProcess {
export class ChildProcess implements Disposable {
get stdin(): WritableStream<Uint8Array>;
get stdout(): ReadableStream<Uint8Array>;
get stderr(): ReadableStream<Uint8Array>;
Expand All @@ -4307,6 +4311,8 @@ declare namespace Deno {
/** Ensure that the status of the child process does not block the Deno
* process from exiting. */
unref(): void;

[Symbol.dispose](): void;
}

/**
Expand Down Expand Up @@ -5258,7 +5264,7 @@ declare namespace Deno {
* requests on the HTTP server connection.
*
* @category HTTP Server */
export interface HttpConn extends AsyncIterable<RequestEvent> {
export interface HttpConn extends AsyncIterable<RequestEvent>, Disposable {
/** The resource ID associated with this connection. Generally users do not
* need to be aware of this identifier. */
readonly rid: number;
Expand Down Expand Up @@ -5911,7 +5917,7 @@ declare namespace Deno {
*
* @category HTTP Server
*/
export interface HttpServer {
export interface HttpServer extends Disposable {
/** A promise that resolves once server finishes - eg. when aborted using
* the signal passed to {@linkcode ServeOptions.signal}.
*/
Expand Down
4 changes: 3 additions & 1 deletion cli/tsc/dts/lib.deno.unstable.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1724,7 +1724,7 @@ declare namespace Deno {
*
* @category KV
*/
export class Kv {
export class Kv implements Disposable {
/**
* Retrieve the value and versionstamp for the given key from the database
* in the form of a {@linkcode Deno.KvEntryMaybe}. If no value exists for
Expand Down Expand Up @@ -1920,6 +1920,8 @@ declare namespace Deno {
* operations immediately.
*/
close(): void;

[Symbol.dispose](): void;
}

/** **UNSTABLE**: New API, yet to be vetted.
Expand Down
6 changes: 5 additions & 1 deletion ext/fs/30_fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import {
ReadableStreamPrototype,
writableStreamForRid,
} from "ext:deno_web/06_streams.js";
import { pathFromURL } from "ext:deno_web/00_infra.js";
import { pathFromURL, SymbolDispose } from "ext:deno_web/00_infra.js";

function chmodSync(path, mode) {
ops.op_fs_chmod_sync(pathFromURL(path), mode);
Expand Down Expand Up @@ -669,6 +669,10 @@ class FsFile {
}
return this.#writable;
}

[SymbolDispose]() {
core.tryClose(this.rid);
}
}

function checkOpenOptions(options) {
Expand Down
22 changes: 15 additions & 7 deletions ext/http/00_serve.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import {
} from "ext:deno_web/06_streams.js";
import { listen, listenOptionApiName, TcpConn } from "ext:deno_net/01_net.js";
import { listenTls } from "ext:deno_net/02_tls.js";
import { SymbolAsyncDispose } from "ext:deno_web/00_infra.js";
const {
ArrayPrototypePush,
ObjectHasOwn,
Expand Down Expand Up @@ -343,6 +344,7 @@ class CallbackContext {
fallbackHost;
serverRid;
closed;
/** @type {Promise<void> | undefined} */
closing;
listener;

Expand Down Expand Up @@ -671,22 +673,25 @@ function serveHttpOn(context, callback) {
PromisePrototypeCatch(callback(req), promiseErrorHandler);
}

if (!context.closed && !context.closing) {
context.closed = true;
await op_http_close(rid, false);
if (!context.closing && !context.closed) {
context.closing = op_http_close(rid, false);
context.close();
}

await context.closing;
context.close();
context.closed = true;
})();

return {
finished,
async shutdown() {
if (!context.closed && !context.closing) {
if (!context.closing && !context.closed) {
// Shut this HTTP server down gracefully
context.closing = true;
await op_http_close(context.serverRid, true);
context.closed = true;
context.closing = op_http_close(rid, true);
bartlomieju marked this conversation as resolved.
Show resolved Hide resolved
}
await context.closing;
context.closed = true;
},
ref() {
ref = true;
Expand All @@ -700,6 +705,9 @@ function serveHttpOn(context, callback) {
core.unrefOp(currentPromise[promiseIdSymbol]);
}
},
[SymbolAsyncDispose]() {
return this.shutdown();
},
};
}

Expand Down
27 changes: 22 additions & 5 deletions ext/http/01_http.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import {
ReadableStreamPrototype,
} from "ext:deno_web/06_streams.js";
import { serve } from "ext:deno_http/00_serve.js";
import { SymbolDispose } from "ext:deno_web/00_infra.js";
const {
ArrayPrototypeIncludes,
ArrayPrototypeMap,
Expand All @@ -69,6 +70,9 @@ const {
const connErrorSymbol = Symbol("connError");
const _deferred = Symbol("upgradeHttpDeferred");

/** @type {(self: HttpConn, rid: number) => boolean} */
let deleteManagedResource;

class HttpConn {
#rid = 0;
#closed = false;
Expand All @@ -79,7 +83,12 @@ class HttpConn {
// that were created during lifecycle of this request.
// When the connection is closed these resources should be closed
// as well.
managedResources = new SafeSet();
#managedResources = new SafeSet();

static {
deleteManagedResource = (self, rid) =>
SetPrototypeDelete(self.#managedResources, rid);
}

constructor(rid, remoteAddr, localAddr) {
this.#rid = rid;
Expand Down Expand Up @@ -123,7 +132,7 @@ class HttpConn {
}

const { 0: streamRid, 1: method, 2: url } = nextRequest;
SetPrototypeAdd(this.managedResources, streamRid);
SetPrototypeAdd(this.#managedResources, streamRid);
Copy link
Contributor

Choose a reason for hiding this comment

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

is the use of SetPrototype* functions necessary in this class if managedResources is a SafeSet? 🤔


/** @type {ReadableStream<Uint8Array> | undefined} */
let body = null;
Expand Down Expand Up @@ -167,13 +176,21 @@ class HttpConn {
if (!this.#closed) {
this.#closed = true;
core.close(this.#rid);
for (const rid of new SafeSetIterator(this.managedResources)) {
SetPrototypeDelete(this.managedResources, rid);
for (const rid of new SafeSetIterator(this.#managedResources)) {
SetPrototypeDelete(this.#managedResources, rid);
core.close(rid);
}
}
}

[SymbolDispose]() {
core.tryClose(this.#rid);
for (const rid of new SafeSetIterator(this.#managedResources)) {
SetPrototypeDelete(this.#managedResources, rid);
core.tryClose(rid);
}
}

[SymbolAsyncIterator]() {
// deno-lint-ignore no-this-alias
const httpConn = this;
Expand Down Expand Up @@ -395,7 +412,7 @@ function createRespondWith(
abortController.abort(error);
throw error;
} finally {
if (SetPrototypeDelete(httpConn.managedResources, streamRid)) {
if (deleteManagedResource(httpConn, streamRid)) {
core.close(streamRid);
}
}
Expand Down
5 changes: 5 additions & 0 deletions ext/kv/01_db.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const {
SymbolToStringTag,
Uint8ArrayPrototype,
} = globalThis.__bootstrap.primordials;
import { SymbolDispose } from "ext:deno_web/00_infra.js";
const core = Deno.core;
const ops = core.ops;

Expand Down Expand Up @@ -299,6 +300,10 @@ class Kv {
close() {
core.close(this.#rid);
}

[SymbolDispose]() {
core.tryClose(this.#rid);
}
}

class AtomicOperation {
Expand Down
2 changes: 1 addition & 1 deletion ext/kv/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ const MAX_TOTAL_MUTATION_SIZE_BYTES: usize = 800 * 1024;
const MAX_TOTAL_KEY_SIZE_BYTES: usize = 80 * 1024;

deno_core::extension!(deno_kv,
deps = [ deno_console ],
deps = [ deno_console, deno_web ],
parameters = [ DBH: DatabaseHandler ],
ops = [
op_kv_database_open<DBH>,
Expand Down
10 changes: 10 additions & 0 deletions ext/net/01_net.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import {
writableStreamForRid,
} from "ext:deno_web/06_streams.js";
import * as abortSignal from "ext:deno_web/03_abort_signal.js";
import { SymbolDispose } from "ext:deno_web/00_infra.js";

const primordials = globalThis.__bootstrap.primordials;
const {
ArrayPrototypeFilter,
Expand Down Expand Up @@ -160,6 +162,10 @@ class Conn {
(id) => core.unrefOp(id),
);
}

[SymbolDispose]() {
core.tryClose(this.#rid);
}
}

class TcpConn extends Conn {
Expand Down Expand Up @@ -249,6 +255,10 @@ class Listener {
core.close(this.rid);
}

[SymbolDispose]() {
core.tryClose(this.#rid);
}

[SymbolAsyncIterator]() {
return this;
}
Expand Down
Loading
Loading