Skip to content

Commit

Permalink
fix(deno): close all subprocess resources (#1238)
Browse files Browse the repository at this point in the history
* refactor(deno): use `deno test` runner

This commit switches the custom test runner to the built in Deno one.

Tests in a single module are now run in series, but if more tests are
added they can be split across files and the upcoming `--jobs` arg can
be used for parallization.

* fix(deno): close all subprocess resources

This commit fixes lib/deno/mod.ts to close all subprocess resources on
cleanup, so stdout and stdin, not just the child process itself.

This commit also changes `esbuild.stop` and `esbuild.initalize` to be
callable multiple times in the same isolate.

* build: update CI to use denoland/setup-deno action
  • Loading branch information
lucacasonato authored May 6, 2021
1 parent ec557e1 commit 7c3ff95
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 84 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ jobs:
node-version: 14

- name: Setup Deno 1.x
uses: denolib/setup-deno@v2
uses: denoland/setup-deno@main
with:
deno-version: v1.x

Expand Down
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ test-wasm-browser: platform-wasm | scripts/browser/node_modules
cd scripts/browser && node browser-tests.js

test-deno: esbuild platform-deno
ESBUILD_BINARY_PATH="$(shell pwd)/esbuild" deno run --allow-run --allow-env --allow-net --allow-read --allow-write scripts/deno-tests.js
ESBUILD_BINARY_PATH="$(shell pwd)/esbuild" deno test --allow-run --allow-env --allow-net --allow-read --allow-write --no-check scripts/deno-tests.js

register-test: cmd/esbuild/version.go | scripts/node_modules
cd npm/esbuild && npm version "$(ESBUILD_VERSION)" --allow-same-version
Expand Down
13 changes: 13 additions & 0 deletions lib/deno/mod.ts
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,13 @@ let ensureServiceIsRunning = (): Promise<Service> => {
})

stopService = () => {
// Close all resources related to the subprocess.
child.stdin.close()
child.stdout.close()
child.close()
initializeWasCalled = false;
longLivedService = undefined;
stopService = undefined
}

let writeQueue: Uint8Array[] = []
Expand Down Expand Up @@ -212,6 +218,13 @@ let ensureServiceIsRunning = (): Promise<Service> => {
readFromStdout(stdoutBuffer.subarray(0, n))
readMoreStdout()
}
}).catch(e => {
if (e instanceof Deno.errors.Interrupted || e instanceof Deno.errors.BadResource) {
// ignore the error if read was interrupted (stdout was closed)
afterClose()
} else {
throw e;
}
})
readMoreStdout()

Expand Down
141 changes: 59 additions & 82 deletions scripts/deno-tests.js
Original file line number Diff line number Diff line change
@@ -1,97 +1,74 @@
// To run this, you must first build the Deno package with "make platform-deno"
import * as esbuild from '../deno/mod.js'
import * as path from 'https://deno.land/std@0.93.0/path/mod.ts'
import * as asserts from 'https://deno.land/std@0.93.0/testing/asserts.ts'
import * as path from 'https://deno.land/std@0.95.0/path/mod.ts'
import * as asserts from 'https://deno.land/std@0.95.0/testing/asserts.ts'

const rootTestDir = path.join(path.dirname(path.fromFileUrl(import.meta.url)), '.deno-tests')

let tests = {
async basicBuild({ testDir }) {
const input = path.join(testDir, 'in.ts')
const dep = path.join(testDir, 'dep.ts')
const output = path.join(testDir, 'out.ts')
await Deno.writeTextFile(input, 'import dep from "./dep.ts"; export default dep === 123')
await Deno.writeTextFile(dep, 'export default 123')
await esbuild.build({
entryPoints: [input],
bundle: true,
outfile: output,
format: 'esm',
})
const result = await import(path.toFileUrl(output))
asserts.assertStrictEquals(result.default, true)
},

async largeBuild() {
// This should be large enough to be bigger than Deno's write buffer
let x = '0'
for (let i = 0; i < 1000; i++)x += '+' + i
x += ','
let y = 'return['
for (let i = 0; i < 1000; i++)y += x
y += ']'
const result = await esbuild.build({
stdin: {
contents: y,
},
write: false,
minify: true,
})
asserts.assertStrictEquals(result.outputFiles[0].text, y.slice(0, -2) + '];\n')
},

async basicTransform() {
const ts = 'let x: number = 1+2'
const result = await esbuild.transform(ts, { loader: 'ts' })
asserts.assertStrictEquals(result.code, 'let x = 1 + 2;\n')
},
try {
Deno.removeSync(rootTestDir, { recursive: true })
} catch {
}
Deno.mkdirSync(rootTestDir, { recursive: true })

async largeTransform() {
// This should be large enough to use the file system passing optimization
let x = '0'
for (let i = 0; i < 1000; i++)x += '+' + i
x += ','
let y = 'return['
for (let i = 0; i < 1000; i++)y += x
y += ']'
const result = await esbuild.transform(y, { minify: true })
asserts.assertStrictEquals(result.code, y.slice(0, -2) + '];\n')
},
function test(name, fn) {
let testDir = path.join(rootTestDir, name)
Deno.test(name, async () => {
await Deno.mkdir(testDir, { recursive: true })
try {
await fn({ testDir })
} finally {
await Deno.remove(testDir, { recursive: true }).catch(() => null)
esbuild.stop()
}
})
}

async function main() {
window.addEventListener("unload", () => {
try {
Deno.removeSync(rootTestDir, { recursive: true })
} catch {
// root test dir possibly already removed, so ignore
}
Deno.mkdirSync(rootTestDir, { recursive: true })
})

// Run all tests concurrently
const runTest = async ([name, fn]) => {
let testDir = path.join(rootTestDir, name)
try {
await Deno.mkdir(testDir, { recursive: true })
await fn({ testDir })
await Deno.remove(testDir, { recursive: true }).catch(() => null)
return true
} catch (e) {
console.error(`❌ ${name}: ${e && e.stack || e}`)
return false
}
}
const allTestsPassed = (await Promise.all(Object.entries(tests).map(runTest))).every(success => success)
esbuild.stop()
test("basicBuild", async ({ testDir }) => {
const input = path.join(testDir, 'in.ts')
const dep = path.join(testDir, 'dep.ts')
const output = path.join(testDir, 'out.ts')
await Deno.writeTextFile(input, 'import dep from "./dep.ts"; export default dep === 123')
await Deno.writeTextFile(dep, 'export default 123')
await esbuild.build({
entryPoints: [input],
bundle: true,
outfile: output,
format: 'esm',
})
const result = await import(path.toFileUrl(output))
asserts.assertStrictEquals(result.default, true)
})

if (!allTestsPassed) {
console.error(`❌ deno tests failed`)
Deno.exit(1)
} else {
console.log(`✅ deno tests passed`)
try {
Deno.removeSync(rootTestDir, { recursive: true })
} catch {
}
}
}
test("basicTransform", async () => {
const ts = 'let x: number = 1+2'
const result = await esbuild.transform(ts, { loader: 'ts' })
asserts.assertStrictEquals(result.code, 'let x = 1 + 2;\n')
})

test("largeTransform", async () => {
// This should be large enough to be bigger than Deno's write buffer
let x = '0'
for (let i = 0; i < 1000; i++)x += '+' + i
x += ','
let y = 'return['
for (let i = 0; i < 1000; i++)y += x
y += ']'
const result = await esbuild.build({
stdin: {
contents: y,
},
write: false,
minify: true,
})
asserts.assertStrictEquals(result.outputFiles[0].text, y.slice(0, -2) + '];\n')
})

await main()

0 comments on commit 7c3ff95

Please sign in to comment.