Skip to content

Commit

Permalink
Merge pull request #64 from tshino/fix-wrap-deadlock
Browse files Browse the repository at this point in the history 
Fix wrap command deadlock
  • Loading branch information
@tshino
tshino authored Feb 27, 2022
2 parents 8c7ebbf + dc05039 commit fdd1627
Show file tree
Hide file tree
Showing 8 changed files with 69 additions and 28 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ All notable changes to the Keyboard Macro Bata extension will be documented in t
### [Unreleased]
- Documentation:
- Updated [DESIGN.md](./DESIGN.md).
- Fix
- Fixed: Deadlock happens if wrap invokes wrap [#63](https://github.com/tshino/vscode-kb-macro/issues/63)
- Internal
- Fixed: Editors remain open after testing [#55](https://github.com/tshino/vscode-kb-macro/issues/55)
- Fixed: gen_wrapper.js and verify_wrapper.js end with exit code 0 even when it fails [#61](https://github.com/tshino/vscode-kb-macro/issues/61)
Expand Down
12 changes: 11 additions & 1 deletion src/keyboard_macro.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,12 +249,15 @@ const KeyboardMacro = function({ awaitController }) {
// greater value leads to too many queued and delayed command execution.
// See: https://github.com/tshino/vscode-kb-macro/pull/32
const WrapQueueSize = 2;
const wrap = reentrantGuard.makeQueueableCommand(async function(args) {
const wrapSync = reentrantGuard.makeQueueableCommand(async function(args) {
if (recording) {
const spec = util.makeCommandSpec(args);
if (!spec) {
return;
}
if (spec.command === 'kb-macro.wrap') {
return;
}
if (onBeginWrappedCommandCallback) {
onBeginWrappedCommandCallback();
}
Expand All @@ -271,6 +274,12 @@ const KeyboardMacro = function({ awaitController }) {
}
}, { queueSize: WrapQueueSize });

const wrap = function(args) {
// Discard the returned Promise.
// See https://github.com/tshino/vscode-kb-macro/issues/63
wrapSync(args);
};

return {
RecordingStateReason,
PlaybackStateReason,
Expand All @@ -289,6 +298,7 @@ const KeyboardMacro = function({ awaitController }) {
abortPlayback,
repeatPlayback,
repeatPlaybackTillEndOfFile,
wrapSync,
wrap,

// testing purpose only
Expand Down
67 changes: 48 additions & 19 deletions test/suite/keyboard_macro.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ describe('KeybaordMacro', () => {
keyboardMacro.onEndWrappedCommand(() => { logs.push('end'); });

keyboardMacro.startRecording();
await keyboardMacro.wrap({ command: 'internal:log' });
await keyboardMacro.wrapSync({ command: 'internal:log' });
keyboardMacro.finishRecording();

assert.deepStrictEqual(logs, [ 'begin', 'invoked', 'end' ]);
Expand Down Expand Up @@ -640,7 +640,7 @@ describe('KeybaordMacro', () => {
});
it('should invoke and record specified command', async () => {
keyboardMacro.startRecording();
await keyboardMacro.wrap({ command: 'internal:log' });
await keyboardMacro.wrapSync({ command: 'internal:log' });
keyboardMacro.finishRecording();

assert.deepStrictEqual(logs, [ 'begin', 'end' ]);
Expand All @@ -649,16 +649,16 @@ describe('KeybaordMacro', () => {
]);
});
it('should not invoke specified command if not recording', async () => {
await keyboardMacro.wrap({ command: 'internal:log' });
await keyboardMacro.wrapSync({ command: 'internal:log' });

assert.deepStrictEqual(logs, []);
});
it('should not crash even if the argument is invalid', async () => {
keyboardMacro.startRecording();
await keyboardMacro.wrap({ command: '' });
await keyboardMacro.wrap({ command: 'INVALID' });
await keyboardMacro.wrap({ });
await keyboardMacro.wrap();
await keyboardMacro.wrapSync({ command: '' });
await keyboardMacro.wrapSync({ command: 'INVALID' });
await keyboardMacro.wrapSync({ });
await keyboardMacro.wrapSync();
keyboardMacro.finishRecording();

assert.deepStrictEqual(logs, []);
Expand All @@ -667,8 +667,8 @@ describe('KeybaordMacro', () => {
});
it('should invoke and record specified command synchronously', async () => {
keyboardMacro.startRecording();
await keyboardMacro.wrap({ command: 'internal:log' });
await keyboardMacro.wrap({ command: 'internal:log', args: { test: '1' } });
await keyboardMacro.wrapSync({ command: 'internal:log' });
await keyboardMacro.wrapSync({ command: 'internal:log', args: { test: '1' } });
keyboardMacro.finishRecording();

assert.deepStrictEqual(logs, [ 'begin', 'end', 'begin', 'end' ]);
Expand All @@ -679,8 +679,8 @@ describe('KeybaordMacro', () => {
});
it('should enqueue and serialize concurrent calls', async () => {
keyboardMacro.startRecording();
const promise1 = keyboardMacro.wrap({ command: 'internal:log' });
const promise2 = keyboardMacro.wrap({ command: 'internal:log' });
const promise1 = keyboardMacro.wrapSync({ command: 'internal:log' });
const promise2 = keyboardMacro.wrapSync({ command: 'internal:log' });
await Promise.all([promise1, promise2]);
keyboardMacro.finishRecording();

Expand All @@ -696,12 +696,12 @@ describe('KeybaordMacro', () => {
it('should be able to enqueue and serialize concurrent call up to WrapQueueSize', async () => {
keyboardMacro.startRecording();
const promises = [];
promises.push(keyboardMacro.wrap({ command: 'internal:log' })); // (1)
promises.push(keyboardMacro.wrapSync({ command: 'internal:log' })); // (1)
for (let i = 0; i < keyboardMacro.WrapQueueSize; i++) {
promises.push(keyboardMacro.wrap({ command: 'internal:log' })); // (2) to (WrapQueueSize + 1)
promises.push(keyboardMacro.wrapSync({ command: 'internal:log' })); // (2) to (WrapQueueSize + 1)
}
await Promise.all(promises);
await keyboardMacro.wrap({ command: 'internal:log' }); // (WrapQueueSize + 2)
await keyboardMacro.wrapSync({ command: 'internal:log' }); // (WrapQueueSize + 2)
keyboardMacro.finishRecording();

const expectedLog = Array(keyboardMacro.WrapQueueSize + 2).fill(['begin', 'end']).flat();
Expand All @@ -712,12 +712,12 @@ describe('KeybaordMacro', () => {
it('should overflow when over WrapQueueSize concurrent calls made', async () => {
keyboardMacro.startRecording();
const promises = [];
promises.push(keyboardMacro.wrap({ command: 'internal:log' })); // (1)
promises.push(keyboardMacro.wrapSync({ command: 'internal:log' })); // (1)
for (let i = 0; i < keyboardMacro.WrapQueueSize + 1; i++) { // <-- PLUS ONE!
promises.push(keyboardMacro.wrap({ command: 'internal:log' })); // (2) to (WrapQueueSize + 2)
promises.push(keyboardMacro.wrapSync({ command: 'internal:log' })); // (2) to (WrapQueueSize + 2)
}
await Promise.all(promises);
await keyboardMacro.wrap({ command: 'internal:log' }); // (WrapQueueSize + 3)
await keyboardMacro.wrapSync({ command: 'internal:log' }); // (WrapQueueSize + 3)
keyboardMacro.finishRecording();

const expectedLog = Array(keyboardMacro.WrapQueueSize + 2).fill(['begin', 'end']).flat();
Expand All @@ -727,7 +727,7 @@ describe('KeybaordMacro', () => {
});
it('should prevent other commands from interrupting wrap command (cancelRecording)', async () => {
keyboardMacro.startRecording();
const promise1 = keyboardMacro.wrap({ command: 'internal:log' });
const promise1 = keyboardMacro.wrapSync({ command: 'internal:log' });
keyboardMacro.cancelRecording(); // <--
await promise1;

Expand All @@ -739,7 +739,7 @@ describe('KeybaordMacro', () => {
});
it('should prevent other commands from interrupting wrap command (finishRecording)', async () => {
keyboardMacro.startRecording();
const promise1 = keyboardMacro.wrap({ command: 'internal:log' });
const promise1 = keyboardMacro.wrapSync({ command: 'internal:log' });
keyboardMacro.finishRecording(); // <--
await promise1;

Expand All @@ -749,5 +749,34 @@ describe('KeybaordMacro', () => {
]);
assert.strictEqual(keyboardMacro.isRecording(), true);
});
it('should prevent direct recursive calls', async () => {
keyboardMacro.startRecording();
await keyboardMacro.wrapSync({
command: 'kb-macro.wrap',
args: { command: 'internal:log' }
});
keyboardMacro.finishRecording();

assert.deepStrictEqual(logs, []);
assert.deepStrictEqual(keyboardMacro.getCurrentSequence(), []);
});
it('should prevent indirect recursive calls', async () => {
// For design details, see https://github.com/tshino/vscode-kb-macro/issues/63
keyboardMacro.registerInternalCommand('internal:indirectWrap', async () => {
await vscode.commands.executeCommand('kb-macro.wrap', {
command: 'internal:log'
});
});
keyboardMacro.startRecording();
await keyboardMacro.wrapSync({
command: 'internal:indirectWrap'
});
keyboardMacro.finishRecording();

assert.deepStrictEqual(logs, []);
assert.deepStrictEqual(keyboardMacro.getCurrentSequence(), [
{ command: 'internal:indirectWrap' }
]);
});
});
});
2 changes: 1 addition & 1 deletion test/suite/playback_async_commands.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ describe('Recording and Playback: Asynchronous Commands', () => {
const record = async function(sequence) {
keyboardMacro.startRecording();
for (let i = 0; i < sequence.length; i++) {
await keyboardMacro.wrap(sequence[i]);
await keyboardMacro.wrapSync(sequence[i]);
}
keyboardMacro.finishRecording();
};
Expand Down
2 changes: 1 addition & 1 deletion test/suite/playback_cursor.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ describe('Recording and Playback: Cursor', () => {
const record = async function(sequence) {
keyboardMacro.startRecording();
for (let i = 0; i < sequence.length; i++) {
await keyboardMacro.wrap(sequence[i]);
await keyboardMacro.wrapSync(sequence[i]);
}
keyboardMacro.finishRecording();
};
Expand Down
2 changes: 1 addition & 1 deletion test/suite/playback_edit.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ describe('Recording and Playback: Edit', () => {
const record = async function(sequence) {
keyboardMacro.startRecording();
for (let i = 0; i < sequence.length; i++) {
await keyboardMacro.wrap(sequence[i]);
await keyboardMacro.wrapSync(sequence[i]);
}
keyboardMacro.finishRecording();
};
Expand Down
2 changes: 1 addition & 1 deletion test/suite/playback_repeat.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ describe('Recording and Playback with Repeat', () => {
const record = async function(sequence) {
keyboardMacro.startRecording();
for (let i = 0; i < sequence.length; i++) {
await keyboardMacro.wrap(sequence[i]);
await keyboardMacro.wrapSync(sequence[i]);
}
keyboardMacro.finishRecording();
};
Expand Down
8 changes: 4 additions & 4 deletions test/suite/playback_typing.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -633,7 +633,7 @@ describe('Recording and Playback: Typing', () => {
it('should record and playback pressing Enter key', async () => {
await setSelections([[10, 2]]);
keyboardMacro.startRecording();
await keyboardMacro.wrap(Cmd.Enter);
await keyboardMacro.wrapSync(Cmd.Enter);
keyboardMacro.finishRecording();
assert.deepStrictEqual(getSequence(), [ Cmd.Enter ]);
assert.strictEqual(textEditor.document.lineAt(10).text, 'ab');
Expand All @@ -649,7 +649,7 @@ describe('Recording and Playback: Typing', () => {
it('should record and playback inserting a line break that results auto-indent', async () => {
await setSelections([[20, 4]]);
keyboardMacro.startRecording();
await keyboardMacro.wrap(Cmd.Enter);
await keyboardMacro.wrapSync(Cmd.Enter);
keyboardMacro.finishRecording();
assert.deepStrictEqual(getSequence(), [ Cmd.Enter ]);
assert.strictEqual(textEditor.document.lineAt(20).text, ' ');
Expand All @@ -675,7 +675,7 @@ describe('Recording and Playback: Typing', () => {
it('should record and playback pressing Tab key', async () => {
await setSelections([[14, 0]]);
keyboardMacro.startRecording();
await keyboardMacro.wrap(Cmd.Tab);
await keyboardMacro.wrapSync(Cmd.Tab);
keyboardMacro.finishRecording();
assert.deepStrictEqual(getSequence(), [ Cmd.Tab ]);
assert.strictEqual(textEditor.document.lineAt(14).text, ' abcd');
Expand All @@ -701,7 +701,7 @@ describe('Recording and Playback: Typing', () => {
keyboardMacro.startRecording();
await vscode.commands.executeCommand('type', { text: 'C' });
await vscode.commands.executeCommand('type', { text: 'D' });
await keyboardMacro.wrap(Cmd.Enter);
await keyboardMacro.wrapSync(Cmd.Enter);
await vscode.commands.executeCommand('type', { text: 'A' });
await vscode.commands.executeCommand('type', { text: 'B' });
keyboardMacro.finishRecording();
Expand Down

0 comments on commit fdd1627

Please sign in to comment.