From 7fec6baeea1d69426d79faae45e7a9fd0594fd9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Thu, 7 Nov 2019 12:12:22 +0100 Subject: [PATCH] fix(runtime): make kernel 'load' operation synchronous The `jsii-runtime` process cannot perform async operations while waiting for a (synchronous) callback to return (the response flow is otherwise ambiguous). In order to allow loading of dependencies within a calllback flow, the `load` kernel API needed to be made syncrhonous. The reason why this is needed is because the host languages runtime libraries may be required to load dependencies at the last minute, for it may not be possible to determine what dependencies are required before a callback is executed. Introduced a (unit) test to confirm the `jsii-runtime` is indeed able to perform `load` operations within a callback context. --- packages/jsii-kernel/lib/kernel.ts | 22 ++--- packages/jsii-kernel/test/kernel.test.ts | 17 ++-- packages/jsii-runtime/.gitignore | 2 + packages/jsii-runtime/.npmignore | 2 + packages/jsii-runtime/lib/host.ts | 7 ++ packages/jsii-runtime/lib/index.ts | 4 +- packages/jsii-runtime/package.json | 30 +++++- .../__snapshots__/kernel-host.test.js.snap | 66 +++++++++++++ .../jsii-runtime/test/kernel-host.test.ts | 87 ++++++++++++++++ packages/jsii-runtime/test/playback-test.sh | 60 ------------ packages/jsii-runtime/test/playback.test.ts | 98 +++++++++++++++++++ .../test/{stress-test.ts => stress.test.ts} | 11 ++- 12 files changed, 318 insertions(+), 88 deletions(-) create mode 100644 packages/jsii-runtime/test/__snapshots__/kernel-host.test.js.snap create mode 100644 packages/jsii-runtime/test/kernel-host.test.ts delete mode 100755 packages/jsii-runtime/test/playback-test.sh create mode 100644 packages/jsii-runtime/test/playback.test.ts rename packages/jsii-runtime/test/{stress-test.ts => stress.test.ts} (60%) diff --git a/packages/jsii-kernel/lib/kernel.ts b/packages/jsii-kernel/lib/kernel.ts index 1648fe16f8..063f5edf48 100644 --- a/packages/jsii-kernel/lib/kernel.ts +++ b/packages/jsii-kernel/lib/kernel.ts @@ -56,7 +56,7 @@ export class Kernel { }); } - public async load(req: api.LoadRequest): Promise { + public load(req: api.LoadRequest): api.LoadResponse { this._debug('load', req); if ('assembly' in req) { @@ -64,8 +64,8 @@ export class Kernel { } if (!this.installDir) { - this.installDir = await fs.mkdtemp(path.join(os.tmpdir(), 'jsii-kernel-')); - await fs.mkdirp(path.join(this.installDir, 'node_modules')); + this.installDir = fs.mkdtempSync(path.join(os.tmpdir(), 'jsii-kernel-')); + fs.mkdirpSync(path.join(this.installDir, 'node_modules')); this._debug('creating jsii-kernel modules workdir:', this.installDir); process.on('exit', () => { @@ -81,9 +81,9 @@ export class Kernel { // check if we already have such a module const packageDir = path.join(this.installDir, 'node_modules', pkgname); - if (await fs.pathExists(packageDir)) { + if (fs.pathExistsSync(packageDir)) { // module exists, verify version - const epkg = await fs.readJson(path.join(packageDir, 'package.json')); + const epkg = fs.readJsonSync(path.join(packageDir, 'package.json')); if (epkg.version !== pkgver) { throw new Error(`Multiple versions ${pkgver} and ${epkg.version} of the ` + `package '${pkgname}' cannot be loaded together since this is unsupported by ` @@ -101,19 +101,19 @@ export class Kernel { } // untar the archive to a staging directory, read the jsii spec from it // and then move it to the node_modules directory of the kernel. - const staging = await fs.mkdtemp(path.join(os.tmpdir(), 'jsii-kernel-install-staging-')); + const staging = fs.mkdtempSync(path.join(os.tmpdir(), 'jsii-kernel-install-staging-')); try { - await tar.extract({ strict: true, file: req.tarball, cwd: staging }); + tar.extract({ strict: true, file: req.tarball, cwd: staging, sync: true }); // read .jsii metadata from the root of the package const jsiiMetadataFile = path.join(staging, 'package', spec.SPEC_FILE_NAME); - if (!await fs.pathExists(jsiiMetadataFile)) { + if (!fs.pathExistsSync(jsiiMetadataFile)) { throw new Error(`Package tarball ${req.tarball} must have a file named ${spec.SPEC_FILE_NAME} at the root`); } - const assmSpec = await fs.readJson(jsiiMetadataFile) as spec.Assembly; + const assmSpec = fs.readJsonSync(jsiiMetadataFile) as spec.Assembly; // "install" to "node_modules" directory - await fs.move(path.join(staging, 'package'), packageDir); + fs.moveSync(path.join(staging, 'package'), packageDir); // load the module and capture it's closure const closure = this._execute(`require(String.raw\`${packageDir}\`)`, packageDir); @@ -126,7 +126,7 @@ export class Kernel { }; } finally { this._debug('removing staging directory:', staging); - await fs.remove(staging); + fs.removeSync(staging); } } diff --git a/packages/jsii-kernel/test/kernel.test.ts b/packages/jsii-kernel/test/kernel.test.ts index d156392e98..c1ab2f4025 100644 --- a/packages/jsii-kernel/test/kernel.test.ts +++ b/packages/jsii-kernel/test/kernel.test.ts @@ -814,17 +814,18 @@ defineTest('fails: static methods - not static', (sandbox) => { }); defineTest('loading a module twice idepotently succeeds', async (sandbox) => { - await sandbox.load({ + sandbox.load({ tarball: await preparePackage('jsii-calc', false), name: 'jsii-calc', version: calcVersion }); }); -defineTest('fails if trying to load two different versions of the same module', async (sandbox) => - expect(sandbox.load({ tarball: await preparePackage('jsii-calc', false), name: 'jsii-calc', version: '99.999.9' })) - .rejects.toThrow(/Multiple versions .+ and .+ of the package 'jsii-calc' cannot be loaded together/) -); +defineTest('fails if trying to load two different versions of the same module', async (sandbox) => { + const tarball = await preparePackage('jsii-calc', false); + return expect(() => sandbox.load({ tarball, name: 'jsii-calc', version: '99.999.9' })) + .toThrow(/Multiple versions .+ and .+ of the package 'jsii-calc' cannot be loaded together/) +}); defineTest('node.js standard library', async (sandbox) => { const objref = sandbox.create({ fqn: 'jsii-calc.NodeStandardLibrary' }); @@ -1221,9 +1222,9 @@ async function createCalculatorSandbox(name: string) { sandbox.traceEnabled = `${process.env.JSII_DEBUG}` === '1'; - await sandbox.load({ tarball: await preparePackage('@scope/jsii-calc-base'), name: '@scope/jsii-calc-base', version: calcBaseVersion }); - await sandbox.load({ tarball: await preparePackage('@scope/jsii-calc-lib'), name: '@scope/jsii-calc-lib', version: calcLibVersion }); - await sandbox.load({ tarball: await preparePackage('jsii-calc'), name: 'jsii-calc', version: calcVersion }); + sandbox.load({ tarball: await preparePackage('@scope/jsii-calc-base'), name: '@scope/jsii-calc-base', version: calcBaseVersion }); + sandbox.load({ tarball: await preparePackage('@scope/jsii-calc-lib'), name: '@scope/jsii-calc-lib', version: calcLibVersion }); + sandbox.load({ tarball: await preparePackage('jsii-calc'), name: 'jsii-calc', version: calcVersion }); return sandbox; } diff --git a/packages/jsii-runtime/.gitignore b/packages/jsii-runtime/.gitignore index c09bd5dbf2..7acd56cccb 100644 --- a/packages/jsii-runtime/.gitignore +++ b/packages/jsii-runtime/.gitignore @@ -8,3 +8,5 @@ webpack node_modules/ .nyc_output/ coverage/ + +test/_tarballs/ diff --git a/packages/jsii-runtime/.npmignore b/packages/jsii-runtime/.npmignore index 086fd2d63f..e86c39b712 100644 --- a/packages/jsii-runtime/.npmignore +++ b/packages/jsii-runtime/.npmignore @@ -9,3 +9,5 @@ coverage .eslintrc.* tsconfig.json *.tsbuildinfo + +test/_tarballs/ diff --git a/packages/jsii-runtime/lib/host.ts b/packages/jsii-runtime/lib/host.ts index 04713f655e..e8b396b1fc 100644 --- a/packages/jsii-runtime/lib/host.ts +++ b/packages/jsii-runtime/lib/host.ts @@ -1,8 +1,10 @@ import { api, Kernel } from 'jsii-kernel'; import { Input, InputOutput } from './in-out'; +import { EventEmitter } from 'events'; export class KernelHost { private readonly kernel = new Kernel(this.callbackHandler.bind(this)); + private readonly eventEmitter = new EventEmitter(); public constructor(private readonly inout: InputOutput, private readonly opts: { debug?: boolean, noStack?: boolean } = { }) { this.kernel.traceEnabled = opts.debug ? true : false; @@ -11,6 +13,7 @@ export class KernelHost { public run() { const req = this.inout.read(); if (!req) { + this.eventEmitter.emit('exit'); return; // done } @@ -21,6 +24,10 @@ export class KernelHost { }); } + public on(event: 'exit', listener: () => void) { + this.eventEmitter.on(event, listener); + } + private callbackHandler(callback: api.Callback) { // write a "callback" response, which is a special response that tells diff --git a/packages/jsii-runtime/lib/index.ts b/packages/jsii-runtime/lib/index.ts index 46781d6d7f..d62f2e0ec2 100644 --- a/packages/jsii-runtime/lib/index.ts +++ b/packages/jsii-runtime/lib/index.ts @@ -1,2 +1,2 @@ -// this module doesn't export any symbols - +export * from './host'; +export * from './in-out'; diff --git a/packages/jsii-runtime/package.json b/packages/jsii-runtime/package.json index 3f84032289..f7273b69cf 100644 --- a/packages/jsii-runtime/package.json +++ b/packages/jsii-runtime/package.json @@ -28,8 +28,8 @@ "build": "tsc --build && chmod +x bin/jsii-runtime && /bin/bash ./bundle.sh && npm run lint", "watch": "tsc --build -w", "lint": "eslint . --ext .js,.ts --ignore-path=.gitignore", - "test": "/bin/bash test/playback-test.sh && node test/stress-test.js", - "test:update": "UPDATE_DIFF=1 npm run test", + "test": "jest", + "test:update": "jest -u", "package": "package-js" }, "dependencies": { @@ -39,6 +39,7 @@ "devDependencies": { "@scope/jsii-calc-base": "^0.20.1", "@scope/jsii-calc-lib": "^0.20.1", + "@types/jest": "^24.0.22", "@typescript-eslint/eslint-plugin": "^2.6.1", "@typescript-eslint/parser": "^2.6.1", "eslint": "^6.6.0", @@ -51,5 +52,30 @@ "wasm-loader": "^1.3.0", "webpack": "^4.41.2", "webpack-cli": "^3.3.10" + }, + "jest": { + "collectCoverage": true, + "collectCoverageFrom": [ + "**/bin/**/*.js", + "**/lib/**/*.js" + ], + "coverageReporters": [ + "lcov", + "text" + ], + "coverageThreshold": { + "global": { + "branches": 45, + "statements": 57 + } + }, + "errorOnDeprecated": true, + "setupFilesAfterEnv": [ + "jest-expect-message" + ], + "testEnvironment": "node", + "testMatch": [ + "**/?(*.)+(spec|test).js" + ] } } diff --git a/packages/jsii-runtime/test/__snapshots__/kernel-host.test.js.snap b/packages/jsii-runtime/test/__snapshots__/kernel-host.test.js.snap new file mode 100644 index 0000000000..9abb008173 --- /dev/null +++ b/packages/jsii-runtime/test/__snapshots__/kernel-host.test.js.snap @@ -0,0 +1,66 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`can load libraries from within a callback 1`] = ` +Object { + "ok": Object { + "assembly": "@scope/jsii-calc-base", + "types": "*redacted*", + }, +} +`; + +exports[`can load libraries from within a callback 2`] = ` +Object { + "ok": Object { + "assembly": "@scope/jsii-calc-lib", + "types": "*redacted*", + }, +} +`; + +exports[`can load libraries from within a callback 3`] = ` +Object { + "ok": Object { + "$jsii.byref": "Object@10000", + "$jsii.interfaces": Array [ + "@scope/jsii-calc-lib.IFriendly", + ], + }, +} +`; + +exports[`can load libraries from within a callback 4`] = ` +Object { + "callback": Object { + "cbid": "jsii::callback::20000", + "cookie": undefined, + "invoke": Object { + "args": Array [], + "method": "hello", + "objref": Object { + "$jsii.byref": "Object@10000", + "$jsii.interfaces": Array [ + "@scope/jsii-calc-lib.IFriendly", + ], + }, + }, + }, +} +`; + +exports[`can load libraries from within a callback 5`] = ` +Object { + "ok": Object { + "assembly": "jsii-calc", + "types": "*redacted*", + }, +} +`; + +exports[`can load libraries from within a callback 6`] = ` +Object { + "ok": Object { + "result": "SUCCESS!", + }, +} +`; diff --git a/packages/jsii-runtime/test/kernel-host.test.ts b/packages/jsii-runtime/test/kernel-host.test.ts new file mode 100644 index 0000000000..0a344e01be --- /dev/null +++ b/packages/jsii-runtime/test/kernel-host.test.ts @@ -0,0 +1,87 @@ +import child = require('child_process'); +import fs = require('fs'); +import { api } from 'jsii-kernel'; +import spec = require('jsii-spec'); +import path = require('path'); +import { KernelHost, InputOutput, Input, Output } from '../lib'; + +test('can load libraries from within a callback', () => { + const inout = new TestInputOutput( + [ + { api: 'load', ...loadRequest('@scope/jsii-calc-base') }, + { api: 'load', ...loadRequest('@scope/jsii-calc-lib') }, + { api: 'create', fqn: 'Object', interfaces: ['@scope/jsii-calc-lib.IFriendly'], overrides: [{ method: 'hello' }] }, + { api: 'invoke', objref: { [api.TOKEN_REF]: 'Object@10000' }, method: 'hello' }, + { api: 'load', ...loadRequest('jsii-calc') }, + { complete: { cbid: 'jsii::callback::20000', result: 'SUCCESS!' } }, + ] + ); + const host = new KernelHost(inout, { noStack: true, debug: false }); + return new Promise(ok => { + host.on('exit', () => ok(inout.expectCompleted())); + host.run(); + }); +}); + +class TestInputOutput extends InputOutput { + private readonly inputCommands: Input[]; + + public constructor(inputCommands: Input[], private readonly allowErrors = false) { + super(); + this.inputCommands = inputCommands.reverse(); + } + + public read(): Input | undefined { + return this.inputCommands.pop(); + } + + public write(obj: Output): void { + if (!this.allowErrors) { + expect(obj).not.toHaveProperty('error'); + } + if ('ok' in obj && 'assembly' in obj.ok) { + // Removing the type count as this is subject to change! + (obj.ok as any).types = '*redacted*'; + } + expect(obj).toMatchSnapshot(); + } + + /** + * Validates that all inputs have been consumed, and all expected outputs have been checked. + */ + public expectCompleted(): void { + expect(this.inputCommands).toEqual([]); + } +} + +function loadRequest(library: string): api.LoadRequest { + const assembly = loadAssembly(); + const tarball = path.join(__dirname, '_tarballs', library, `${assembly.fingerprint.replace('/', '_')}.tgz`); + if (!fs.existsSync(tarball)) { + packageLibrary(tarball); + } + return { + name: assembly.name, + version: assembly.version, + tarball, + }; + + function loadAssembly(): spec.Assembly { + const assemblyFile = path.resolve(require.resolve(`${library}/package.json`), '..', '.jsii'); + return JSON.parse(fs.readFileSync(assemblyFile, { encoding: 'utf-8' })); + } + + function packageLibrary(target: string): void { + const targetDir = path.dirname(target); + fs.mkdirSync(targetDir, { recursive: true }); + const result = child.spawnSync('npm', ['pack', path.dirname(require.resolve(`${library}/package.json`))], { cwd: targetDir, stdio: ['inherit', 'pipe', 'pipe'] }); + if (result.error) { + throw result.error; + } + if (result.status !== 0) { + console.error(result.stderr.toString('utf-8')); + throw new Error(`Unable to 'npm pack' ${library}: process ${result.signal != null ? `killed by ${result.signal}` : `exited with code ${result.status}`}`); + } + fs.renameSync(path.join(targetDir, result.stdout.toString('utf-8').trim()), target); + } +} diff --git a/packages/jsii-runtime/test/playback-test.sh b/packages/jsii-runtime/test/playback-test.sh deleted file mode 100755 index 7190e9d2fa..0000000000 --- a/packages/jsii-runtime/test/playback-test.sh +++ /dev/null @@ -1,60 +0,0 @@ -#!/bin/bash -# -# This script executes the kernel unit tests in "recording" mode. In this mode, each test -# will record all kernel inputs (">") and outputs ("<") into a log file. -# -# Then, we take each one of these log files and extract only the inputs (lines that begin with ">"). -# We then execute jsii-runtime and pipe these inputs via STDIN. -# -# jsii-runtime emits the same logging format (inputs and outputs) to STDERR, so we capture -# these lines and then diff them with the expected file to validate. -# -# If the outputs are the same the test passed. -# -set -e -cd $(dirname $0) - -export JSII_RECORD=$(mktemp -d) # recording dir output -export JSII_NOSTACK=1 # stack traces will not match -export JSII_DEBUG=1 # emit debug log from jsii-runtime - -jsii_runtime_program="../webpack/jsii-runtime.js" - -# run jsii-kernel tests and save recordings -( - cd ../../jsii-kernel - jest test/kernel.test.js >& /tmp/test-output || { - cat /tmp/test-output - exit 1 - } -) - -# play back each test into jsii-runtime and compare results -for file in $(ls -1 ${JSII_RECORD}); do - recording="${JSII_RECORD}/${file}" - actual="${recording}.actual" - name="$(basename ${recording} .log)" - - # announce - echo " + ${name}" - - # play back recording into jsii-runtime (responses are ignored) and save all stderr lines - # that start with "> " or "< " to ${actual}, to be diffed with recording. - cat ${recording} | node ${jsii_runtime_program} 2> /tmp/jsii-runtime.stderr 1> /dev/null - cat /tmp/jsii-runtime.stderr | grep '^[<>] ' > ${actual} || { - cat ${recording} | node ${jsii_runtime_program} - exit 1 - } - - # compare expected and actual - if ! diff ${actual} ${recording}; then - echo "=========================================================================" - echo "Expected: ${recording}" - echo "Actual: ${actual}" - echo "=========================================================================" - exit 1 - fi -done - -echo -echo "Recordings: ${JSII_RECORD}" diff --git a/packages/jsii-runtime/test/playback.test.ts b/packages/jsii-runtime/test/playback.test.ts new file mode 100644 index 0000000000..76a1a5c226 --- /dev/null +++ b/packages/jsii-runtime/test/playback.test.ts @@ -0,0 +1,98 @@ +import child = require('child_process'); +import fs = require('fs'); +import os = require('os'); +import path = require('path'); +import process = require('process'); + +import { InputOutput, KernelHost, Input, Output } from '../lib'; + +const recordsDir = createRecords(); +const records = fs.readdirSync(recordsDir).map(file => path.join(recordsDir, file)); + +test('are present', () => { + expect(records).not.toEqual([]); +}); + +describe(`replay records in ${recordsDir}`, () => { + for (const record of records) { + test(path.basename(record, '.log'), () => { + const inout = new PlaybackInputOutput(record); + const host = new KernelHost(inout, { noStack: true, debug: false }); + + return new Promise(ok => { + host.on('exit', () => { + ok(inout.expectCompleted()); + }); + + host.run(); + }); + }); + } +}); + +function createRecords(): string { + const records = fs.mkdtempSync(path.join(os.tmpdir(), 'jsii-kernel.recording.')); + const result = child.spawnSync( + require.resolve('jest/bin/jest'), + [require.resolve('jsii-kernel/test/kernel.test.js')], + { + env: { ...process.env, JSII_RECORD: records, JSII_NOSTACK: '1' }, + shell: true, + stdio: ['inherit', 'pipe', 'pipe'], + cwd: path.resolve(require.resolve('jsii-kernel/test/kernel.test.js'), '..', '..'), + }, + ); + + if (result.error != null) { + throw result.error; + } + + if (result.signal != null || result.status !== 0) { + console.log(result.stdout); + console.error(result.stderr.toString('utf-8')); + } + + if (result.signal != null) { + throw new Error(`Kernel test runner exited with signal: ${result.signal}`); + } + + if (result.status !== 0) { + throw new Error(`Kernel test runner exited with code: ${result.status}`); + } + + return records; +} + +class PlaybackInputOutput extends InputOutput { + public readonly inputCommands: Input[]; + public readonly expectedOutputs: Output[]; + + public constructor(recordPath: string) { + super(); + const inputLines = fs.readFileSync(recordPath, { encoding: 'utf-8' }).split('\n'); + this.inputCommands = inputLines + .filter(line => line.startsWith('>')) + .map(line => JSON.parse(line.substring(1))) + .reverse(); + this.expectedOutputs = inputLines + .filter(line => line.startsWith('<')) + .map(line => JSON.parse(line.substring(1))) + .reverse(); + } + + public read(): Input | undefined { + return this.inputCommands.pop(); + } + + public write(obj: Output): void { + expect(obj).toEqual(this.expectedOutputs.pop()); + } + + /** + * Validates that all inputs have been consumed, and all expected outputs have been checked. + */ + public expectCompleted(): void { + expect(this.inputCommands).toEqual([]); + expect(this.expectedOutputs).toEqual([]); + } +} diff --git a/packages/jsii-runtime/test/stress-test.ts b/packages/jsii-runtime/test/stress.test.ts similarity index 60% rename from packages/jsii-runtime/test/stress-test.ts rename to packages/jsii-runtime/test/stress.test.ts index c83a8cc3ff..6a7885100e 100755 --- a/packages/jsii-runtime/test/stress-test.ts +++ b/packages/jsii-runtime/test/stress.test.ts @@ -1,7 +1,7 @@ import { InputOutput, Input, Output } from '../lib/in-out'; import { KernelHost } from '../lib/host'; -const requestCount = 250000; +const requestCount = 250_000; class FakeInputOutput extends InputOutput { public debug = false; @@ -19,8 +19,9 @@ class FakeInputOutput extends InputOutput { } } -const inout = new FakeInputOutput(); -const host = new KernelHost(inout, { debug: false, noStack: false }); -host.run(); +test(`runtime host is able to action ${requestCount} requests`, () => { + const inout = new FakeInputOutput(); + const host = new KernelHost(inout, { debug: false, noStack: false }); -console.info('jsii-runtime stress test succeeded'); + expect(() => host.run()).not.toThrow(); +});