Skip to content

Commit c68a8a4

Browse files
kevinwang5658kevinwang5658
committed
IPC Bridge bug fixes + refactor + tests
1 parent 1aa7669 commit c68a8a4

4 files changed

Lines changed: 191 additions & 34 deletions

File tree

‎codify-core/.eslintrc.json‎

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,23 @@
11
{
2-
"extends": ["oclif", "oclif-typescript", "prettier"],
2+
"extends": [
3+
"oclif",
4+
"oclif-typescript",
5+
"prettier"
6+
],
37
"rules": {
4-
"object-curly-spacing": ["warn", "always"],
5-
"perfectionist/sort-classes": ["off"],
6-
"quotes": ["error", "single"]
7-
}
8+
"object-curly-spacing": [
9+
"warn",
10+
"always"
11+
],
12+
"perfectionist/sort-classes": [
13+
"off"
14+
],
15+
"quotes": [
16+
"error",
17+
"single"
18+
]
19+
},
20+
"ignorePatterns": [
21+
"*.test.ts"
22+
]
823
}

‎codify-core/package.json‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,13 @@
1515
"@oclif/prettier-config": "^0.2.1",
1616
"@oclif/test": "^3",
1717
"@types/chai": "^4",
18+
"@types/chai-as-promised": "^7.1.7",
1819
"@types/mocha": "^9.0.0",
1920
"@types/mock-fs": "^4.13.3",
2021
"@types/node": "^18",
2122
"@types/semver": "^7.5.4",
2223
"chai": "^4",
24+
"chai-as-promised": "^7.1.1",
2325
"eslint": "^8.51.0",
2426
"eslint-config-oclif": "^5",
2527
"eslint-config-oclif-typescript": "^3",

‎codify-core/src/plugins/ipc-bridge.test.ts‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
import { describe, it } from 'mocha';
2+
import { EventEmitter } from 'node:events';
3+
import { ChildProcess } from 'node:child_process';
4+
5+
import { Readable } from 'stream';
6+
import { PluginIpcBridge } from './ipc-bridge';
7+
import { mock } from 'node:test';
8+
import { expect } from '@oclif/test';
9+
import * as chai from 'chai';
10+
import { AssertionError } from 'chai';
11+
import * as chaiAsPromised from 'chai-as-promised';
12+
13+
describe('Plugin IPC Bridge tests', async () => {
14+
15+
before(() => {
16+
chai.use(chaiAsPromised);
17+
chai.should();
18+
})
19+
20+
const mockChildProcess = () => {
21+
const process = new ChildProcess();
22+
process.stdout = new EventEmitter() as Readable;
23+
process.stderr = new EventEmitter() as Readable
24+
process.send = () => true;
25+
26+
return process;
27+
}
28+
29+
it('send a message', async () => {
30+
const process = mockChildProcess();
31+
const sendMock = mock.method(process, 'send');
32+
33+
const ipcBridge = new PluginIpcBridge(process);
34+
ipcBridge.sendMessage({ cmd: 'message', data: 'data' })
35+
36+
expect(sendMock.mock.calls.length).to.eq(1);
37+
expect(sendMock.mock.calls[0].arguments[0]).to.deep.eq({ cmd: 'message', data: 'data' });
38+
})
39+
40+
it('send a message and receives the response', async () => {
41+
const process = mockChildProcess();
42+
const ipcBridge = new PluginIpcBridge(process);
43+
44+
try {
45+
await Promise.all([
46+
expect(ipcBridge.sendMessageForResult({ cmd: 'message', data: 'data' })).to.eventually.eq('data'),
47+
process.emit('message', { cmd: 'messageResult', data: 'data' }),
48+
]);
49+
} catch (e) {
50+
throw new AssertionError('Failed to receive message');
51+
}
52+
});
53+
54+
it('validates bad responses', async () => {
55+
const process = mockChildProcess();
56+
const ipcBridge = new PluginIpcBridge(process);
57+
58+
try {
59+
await Promise.all([
60+
ipcBridge.sendMessageForResult({ cmd: 'message', data: 'data' }),
61+
process.emit('message', 'data'),
62+
]);
63+
} catch (e) {
64+
expect(e).to.throw
65+
}
66+
});
67+
68+
it('does not leave additional listeners', async () => {
69+
const process = mockChildProcess();
70+
const ipcBridge = new PluginIpcBridge(process);
71+
72+
// NodeJS promise.all is executed in order
73+
await Promise.all([
74+
ipcBridge.sendMessageForResult({ cmd: 'message', data: 'data' }),
75+
expect(process.listeners('message').length).to.eq(1),
76+
process.emit('message', { cmd: 'messageResult', data: 'data' }),
77+
expect(process.listeners('message').length).to.eq(0),
78+
expect(process.stdout!.listeners('data').length).to.eq(0),
79+
expect(process.stderr!.listeners('data').length).to.eq(0),
80+
]);
81+
});
82+
83+
it('does not interfere with existing listeners', async () => {
84+
const process = mockChildProcess();
85+
const ipcBridge = new PluginIpcBridge(process);
86+
process.on('message', () => {
87+
})
88+
89+
await Promise.all([
90+
ipcBridge.sendMessageForResult({ cmd: 'message', data: 'data' }),
91+
expect(process.listeners('message').length).to.eq(2),
92+
process.emit('message', { cmd: 'messageResult', data: 'data' }),
93+
expect(process.listeners('message').length).to.eq(1),
94+
]);
95+
});
96+
97+
it('allows new listeners to be added while waiting for the result', async () => {
98+
const process = mockChildProcess();
99+
const ipcBridge = new PluginIpcBridge(process);
100+
101+
await Promise.all([
102+
ipcBridge.sendMessageForResult({ cmd: 'message', data: 'data' }),
103+
process.on('message', () => {
104+
}),
105+
expect(process.listeners('message').length).to.eq(2),
106+
process.emit('message', { cmd: 'messageResult', data: 'data' }),
107+
expect(process.listeners('message').length).to.eq(1),
108+
]);
109+
});
110+
});

‎codify-core/src/plugins/ipc-bridge.ts‎

Lines changed: 59 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,11 @@ import { config } from '../project-configs';
44
import { validateTypeRecordStringUnknown } from '../utils/validator';
55
import { PluginMessage } from './entities/message';
66

7+
type Resolve = (value: unknown) => void;
8+
type Reject = (reason?: Error) => void;
9+
10+
const resultFunctionName = (cmd: string) => `${cmd}Result`;
11+
712
export class PluginIpcBridge {
813

914
process: ChildProcess;
@@ -28,43 +33,68 @@ export class PluginIpcBridge {
2833

2934
async sendMessageForResult(message: PluginMessage): Promise<unknown> {
3035
return new Promise((resolve, reject) => {
31-
const timer = setTimeout(() => {
32-
this.process.kill();
33-
reject(new Error(`Plugin did not respond in 10s to call: ${message.cmd}`))
34-
}, 10_000);
35-
36-
const errorListener = (error: Buffer) => {
37-
this.process.kill();
38-
reject(error.toString());
39-
}
40-
41-
const messageListener = (incomingMessage: unknown) => {
42-
console.log(incomingMessage);
43-
44-
if (!validateTypeRecordStringUnknown(incomingMessage)) {
45-
return reject(new Error(`Bad message from plugin ${name}. ${JSON.stringify(incomingMessage, null, 2)}`))
46-
}
47-
48-
if (incomingMessage.cmd === this.getResultFunctionName(message.cmd)) {
49-
clearTimeout(timer);
50-
this.process.removeListener('message', messageListener);
51-
this.process.removeListener('error', errorListener);
52-
resolve(incomingMessage.data);
53-
}
54-
};
55-
56-
this.process.on('message', messageListener);
57-
this.process.stderr!.on('data', errorListener);
36+
const handler = new SendMessageForResultHandler(message, this.process, resolve, reject);
37+
38+
this.process.on('message', handler.messageListener);
39+
this.process.stderr!.on('data', handler.reject);
5840
this.process.send(message);
5941
});
6042
}
6143

6244
sendMessage(message: PluginMessage): void {
6345
this.process.send(message);
6446
}
47+
}
6548

66-
private getResultFunctionName(rpcFunctionName: string): string {
67-
return rpcFunctionName + 'Result';
49+
class SendMessageForResultHandler {
50+
messageToSend: PluginMessage;
51+
process: ChildProcess;
52+
promiseResolve: Resolve;
53+
promiseReject: Reject;
54+
timer: NodeJS.Timeout;
55+
56+
constructor(messageToSend: PluginMessage, process: ChildProcess, resolve: Resolve, reject: Reject) {
57+
this.messageToSend = messageToSend;
58+
this.process = process;
59+
this.promiseResolve = resolve;
60+
this.promiseReject = reject;
61+
this.timer = this.setResultTimeout();
6862
}
6963

64+
messageListener = (incomingMessage: unknown) => {
65+
console.log(incomingMessage);
66+
67+
if (!validateTypeRecordStringUnknown(incomingMessage)) {
68+
return this.reject(new Error(`Bad message from plugin. ${JSON.stringify(incomingMessage, null, 2)}`))
69+
}
70+
71+
if (incomingMessage.cmd === resultFunctionName(this.messageToSend.cmd)) {
72+
this.resolve(incomingMessage.data);
73+
}
74+
};
75+
76+
reject = (err: Error) => {
77+
if (this.timer.hasRef()) {
78+
clearTimeout(this.timer);
79+
}
80+
81+
this.process.removeListener('message', this.messageListener);
82+
this.process.stderr!.removeListener('data', this.reject);
83+
this.promiseReject(err);
84+
}
85+
86+
private resolve = (value: unknown) => {
87+
if (this.timer.hasRef()) {
88+
clearTimeout(this.timer);
89+
}
90+
91+
this.process.removeListener('message', this.messageListener);
92+
this.process.stderr!.removeListener('data', this.reject);
93+
this.promiseResolve(value);
94+
}
95+
96+
private setResultTimeout = () => setTimeout(() => {
97+
this.reject(new Error(`Plugin did not respond in 10s to call: ${this.messageToSend.cmd}`))
98+
}, 10_000);
7099
}
100+

0 commit comments

Comments
 (0)