fix(javascript): match resource release methods to the acquired handle (#7773)
* fix(javascript): match resource release methods to the acquired handle
Every acquisition family admitted every release method, so `h.close()`,
`h.destroy()` and `h.end()` each resolved as a complete release of whatever the
binding held. Node exposes a different subset per acquisition, and the rejected
pairs throw at the call site instead.
Measured on Node 24.22.1:
| acquisition | value | release members |
| --- | --- | --- |
| `fs.openSync` | number | none |
| `fs.open` (callback) | undefined | none |
| `fs.createReadStream` | ReadStream | `close`, `destroy` (`end` is the byte-offset field) |
| `fs.createWriteStream` | WriteStream | `close`, `destroy`, `end` |
| `fs/promises.open` | FileHandle | `close` |
| `net.connect` | Socket | `destroy`, `end` |
| `tls.connect` | TLSSocket | `destroy`, `end` |
So `fs.openSync(...)` + `fd.close()`, `createReadStream(...)` + `.end()`,
`net.connect(...)` + `.close()` and the FileHandle cases emitted a `releases` /
`resolved` fact for a call that throws.
Carry the specifier with each acquisition candidate, list the release methods
each pair actually has, and emit no release when the pair is not one of them. A
release stays `complete` only when the binding held exactly one acquisition and
that acquisition admitted the method.
`tls.createConnection` is not a Node export at all: `'createConnection' in
require('node:tls')` is false and the ESM import does not link. Drop it from the
table so a `tls.createConnection` call yields no acquisition.
* chore: retrigger title and readiness checks after the title was corrected
* fix(javascript): cover a mixed-family binding before completing a release
greptile asked for the mixed-family case: a binding that can hold either a
FileHandle or a socket, then calls close(). The FileHandle is the only linked
acquisition and the release stays partial.
That case also showed the `candidates.length === 1` guard was dead: an
acquisition of another family is not in `linked`, so `holdsOnlyAcquisitions`
already returns false for it and the release already stayed partial without the
guard. Removing the guard and keeping the test means the behaviour is now pinned
by a test rather than by two overlapping conditions.
This commit is contained in:
1 parent
a965cbab06
commit
b7442c1958
3 files changed
+196
-8
No files matched your search
@@ -3,9 +3,32 @@ export const BUILTIN_RESOURCE_METHODS = {
|
||||
fs: ["open", "openSync", "createReadStream", "createWriteStream"],
|
||||
"fs/promises": ["open"],
|
||||
net: ["connect", "createConnection"],
|
||||
tls: ["connect", "createConnection"],
|
||||
tls: ["connect"],
|
||||
} as const;
|
||||
|
||||
/**
|
||||
* Release methods the value Node returns from each acquisition actually has.
|
||||
*
|
||||
* `fs.openSync` returns a numeric descriptor and `fs.open` in its callback form
|
||||
* returns `undefined`, so neither has a release method at all; the descriptor is
|
||||
* released through `fs.closeSync`/`fs.close`, which is not a member call. A
|
||||
* `fs.ReadStream` exposes `end` as its byte-offset field, not a method. A
|
||||
* `FileHandle` and a `net.Socket`/`TLSSocket` each expose only their own subset,
|
||||
* so a family-blind release list claims a teardown that throws instead.
|
||||
*/
|
||||
export const BUILTIN_RESOURCE_RELEASE_METHODS = {
|
||||
"fs:open": [],
|
||||
"fs:openSync": [],
|
||||
"fs:createReadStream": ["close", "destroy"],
|
||||
"fs:createWriteStream": ["close", "destroy", "end"],
|
||||
"fs/promises:open": ["close"],
|
||||
"net:connect": ["destroy", "end"],
|
||||
"net:createConnection": ["destroy", "end"],
|
||||
"tls:connect": ["destroy", "end"],
|
||||
} as const satisfies Readonly<
|
||||
Record<string, readonly ("close" | "destroy" | "end")[]>
|
||||
>;
|
||||
|
||||
export const BUILTIN_RESOURCE_SPECIFIERS = Object.keys(
|
||||
BUILTIN_RESOURCE_METHODS,
|
||||
);
|
||||
@@ -0,0 +1,134 @@
|
||||
import { expect, it } from "vitest";
|
||||
|
||||
import { analyzeJavaScriptSemantics } from "./javascriptSemanticAnalysis.js";
|
||||
|
||||
/**
|
||||
* Node's own objects expose different teardown members per acquisition, so a
|
||||
* member call that Node rejects must not be reported as a release. Every row is
|
||||
* a pair that throws at the call site on Node 24.22.1.
|
||||
*/
|
||||
const rejected = [
|
||||
[
|
||||
"a numeric descriptor from fs.openSync",
|
||||
`import { openSync } from "node:fs";
|
||||
const fd = openSync("input.txt", "r");
|
||||
fd.close();`,
|
||||
],
|
||||
[
|
||||
"a numeric descriptor released with destroy",
|
||||
`import { openSync } from "node:fs";
|
||||
const fd = openSync("input.txt", "r");
|
||||
fd.destroy();`,
|
||||
],
|
||||
[
|
||||
"the callback form of fs.open",
|
||||
`import { open } from "node:fs";
|
||||
const pending = open("input.txt", "r", () => {});
|
||||
pending.close();`,
|
||||
],
|
||||
[
|
||||
"a read stream's byte-offset field",
|
||||
`import { createReadStream } from "node:fs";
|
||||
const stream = createReadStream("input.txt");
|
||||
stream.end();`,
|
||||
],
|
||||
[
|
||||
"a FileHandle without a release member",
|
||||
`import { open } from "node:fs/promises";
|
||||
const handle = await open("input.txt");
|
||||
handle.destroy();`,
|
||||
],
|
||||
[
|
||||
"a socket without close",
|
||||
`import { connect } from "node:net";
|
||||
const socket = connect(9);
|
||||
socket.close();`,
|
||||
],
|
||||
[
|
||||
"a TLS socket without close",
|
||||
`import { connect } from "node:tls";
|
||||
const socket = connect(9);
|
||||
socket.close();`,
|
||||
],
|
||||
];
|
||||
|
||||
it.each(rejected)("emits no release for %s", (_name, source) => {
|
||||
expect(
|
||||
analyzeJavaScriptSemantics(source)
|
||||
.resourceOperations.filter(({ kind }) => kind === "release")
|
||||
.map(({ method }) => method),
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
/** The pairs Node does support stay complete, so the filter is pure narrowing. */
|
||||
const accepted = [
|
||||
[
|
||||
"a FileHandle closed",
|
||||
`import { open } from "node:fs/promises";
|
||||
const handle = await open("input.txt");
|
||||
handle.close();`,
|
||||
"close",
|
||||
],
|
||||
[
|
||||
"a read stream destroyed",
|
||||
`import { createReadStream } from "node:fs";
|
||||
const stream = createReadStream("input.txt");
|
||||
stream.destroy();`,
|
||||
"destroy",
|
||||
],
|
||||
[
|
||||
"a socket ended",
|
||||
`import { connect } from "node:net";
|
||||
const socket = connect(9);
|
||||
socket.end();`,
|
||||
"end",
|
||||
],
|
||||
];
|
||||
|
||||
it.each(accepted)("keeps %s complete", (_name, source, method) => {
|
||||
expect(
|
||||
analyzeJavaScriptSemantics(source)
|
||||
.resourceOperations.filter(({ kind }) => kind === "release")
|
||||
.map(({ method: released, resolution }) => ({
|
||||
method: released,
|
||||
resolution,
|
||||
})),
|
||||
).toEqual([{ method, resolution: "complete" }]);
|
||||
});
|
||||
|
||||
/**
|
||||
* A binding that can hold more than one acquisition stays partial even when a
|
||||
* method is admissible for one of them: the release is only certain if the
|
||||
* binding held exactly one acquisition before the filter ran.
|
||||
*/
|
||||
it.each([
|
||||
[
|
||||
"a FileHandle or a socket, released with close",
|
||||
`import { open } from "node:fs/promises";
|
||||
import { connect } from "node:net";
|
||||
let handle;
|
||||
if (flag) { handle = await open("input.txt"); }
|
||||
else { handle = connect(9); }
|
||||
handle.close();`,
|
||||
[{ method: "close", linked: 1, resolution: "partial" }],
|
||||
],
|
||||
[
|
||||
"a read stream or a write stream, released with end",
|
||||
`import { createReadStream, createWriteStream } from "node:fs";
|
||||
let stream;
|
||||
if (flag) { stream = createReadStream("input.txt"); }
|
||||
else { stream = createWriteStream("output.txt"); }
|
||||
stream.end();`,
|
||||
[{ method: "end", linked: 1, resolution: "partial" }],
|
||||
],
|
||||
])("keeps %s partial", (_name, source, expected) => {
|
||||
expect(
|
||||
analyzeJavaScriptSemantics(source)
|
||||
.resourceOperations.filter(({ kind }) => kind === "release")
|
||||
.map(({ method, linkedResourceIds, resolution }) => ({
|
||||
method,
|
||||
linked: linkedResourceIds.length,
|
||||
resolution,
|
||||
})),
|
||||
).toEqual(expected);
|
||||
});
|
||||
@@ -3,6 +3,7 @@ import * as t from "@babel/types";
|
||||
import {
|
||||
BUILTIN_RESOURCE_SPECIFIERS,
|
||||
BUILTIN_RESOURCE_METHODS,
|
||||
BUILTIN_RESOURCE_RELEASE_METHODS,
|
||||
} from "./javascriptBuiltinResourceMethods.js";
|
||||
import type {
|
||||
JavaScriptSemanticCallable,
|
||||
@@ -29,8 +30,23 @@ import { range } from "./javascriptStaticAnalysisHelpers.js";
|
||||
|
||||
const RELEASE_METHODS = ["close", "destroy", "end"] as const;
|
||||
|
||||
/**
|
||||
* Release methods the value from this acquisition actually has. An unlisted
|
||||
* specifier/method pair admits none, so a call to it emits no release fact.
|
||||
*/
|
||||
const releaseMethodsFor = (
|
||||
acquisition: ResourceCandidate,
|
||||
): readonly ("close" | "destroy" | "end")[] => {
|
||||
const key = `${acquisition.specifier}:${acquisition.method}`;
|
||||
const table: Readonly<
|
||||
Record<string, readonly ("close" | "destroy" | "end")[]>
|
||||
> = BUILTIN_RESOURCE_RELEASE_METHODS;
|
||||
return table[key] ?? [];
|
||||
};
|
||||
|
||||
interface ResourceCandidate {
|
||||
readonly node: t.CallExpression | t.OptionalCallExpression;
|
||||
readonly specifier: string;
|
||||
readonly method: JavaScriptSemanticResourceOperation["method"];
|
||||
readonly ownerCallableId: string | null;
|
||||
readonly resultBindingId: string | null;
|
||||
@@ -62,11 +78,12 @@ const collectAcquisitions = (
|
||||
const output: ResourceCandidate[] = [];
|
||||
traverseDataEffects(program, context, (node) => {
|
||||
if (!t.isCallExpression(node) && !t.isOptionalCallExpression(node)) return;
|
||||
const method = acquisitionMethod(node.callee, context.state);
|
||||
if (method === null) return;
|
||||
const reference = acquisitionMethod(node.callee, context.state);
|
||||
if (reference === null) return;
|
||||
output.push({
|
||||
node,
|
||||
method,
|
||||
specifier: reference.specifier,
|
||||
method: reference.method,
|
||||
ownerCallableId: context.callableStack.at(-1) ?? null,
|
||||
resultBindingId: outerDataEffectBinding(node, context),
|
||||
});
|
||||
@@ -102,9 +119,15 @@ const collectReleases = (
|
||||
member.object,
|
||||
member.object.name,
|
||||
);
|
||||
const linked =
|
||||
const candidates =
|
||||
binding === undefined ? [] : (byBinding.get(binding.bindingId) ?? []);
|
||||
if (binding === undefined || linked.length === 0) return;
|
||||
if (binding === undefined || candidates.length === 0) return;
|
||||
const linked = candidates.filter((acquisition) =>
|
||||
releaseMethodsFor(acquisition).includes(method),
|
||||
);
|
||||
// No linked acquisition exposes this release method, so the call is not a
|
||||
// release of it at all: Node throws at the call site instead.
|
||||
if (linked.length === 0) return;
|
||||
output.push({
|
||||
resourceId: `resource:release:${String(node.start ?? -1)}:${String(node.end ?? -1)}`,
|
||||
kind: "release",
|
||||
@@ -169,10 +192,15 @@ const immutableAcquisition = (
|
||||
resolution: "complete",
|
||||
});
|
||||
|
||||
interface AcquisitionReference {
|
||||
readonly specifier: string;
|
||||
readonly method: JavaScriptSemanticResourceOperation["method"];
|
||||
}
|
||||
|
||||
const acquisitionMethod = (
|
||||
callee: t.Node,
|
||||
state: JavaScriptSemanticAnalysisState,
|
||||
): JavaScriptSemanticResourceOperation["method"] | null => {
|
||||
): AcquisitionReference | null => {
|
||||
for (const specifier of BUILTIN_RESOURCE_SPECIFIERS) {
|
||||
const method = builtinDataEffectMethod(callee, state, [specifier]);
|
||||
if (
|
||||
@@ -183,7 +211,10 @@ const acquisitionMethod = (
|
||||
] as readonly string[]
|
||||
).includes(method)
|
||||
)
|
||||
return method as JavaScriptSemanticResourceOperation["method"];
|
||||
return {
|
||||
specifier,
|
||||
method: method as JavaScriptSemanticResourceOperation["method"],
|
||||
};
|
||||
}
|
||||
return null;
|
||||
};
|
||||
|
||||
Reference in new issue
Block a user