From b55b2f3ff7b8463038adf0a31b2ffc44b31feb7e Mon Sep 17 00:00:00 2001 From: tshino Date: Sat, 26 Feb 2022 00:08:17 +0900 Subject: [PATCH 1/3] Fix wrap command deadlock --- src/keyboard_macro.js | 3 +++ test/suite/keyboard_macro.test.js | 11 +++++++++++ 2 files changed, 14 insertions(+) diff --git a/src/keyboard_macro.js b/src/keyboard_macro.js index f27bf46b..2007b18a 100644 --- a/src/keyboard_macro.js +++ b/src/keyboard_macro.js @@ -255,6 +255,9 @@ const KeyboardMacro = function({ awaitController }) { if (!spec) { return; } + if (spec.command === 'kb-macro.wrap') { + return; + } if (onBeginWrappedCommandCallback) { onBeginWrappedCommandCallback(); } diff --git a/test/suite/keyboard_macro.test.js b/test/suite/keyboard_macro.test.js index 67ea3962..cfd35755 100644 --- a/test/suite/keyboard_macro.test.js +++ b/test/suite/keyboard_macro.test.js @@ -749,5 +749,16 @@ describe('KeybaordMacro', () => { ]); assert.strictEqual(keyboardMacro.isRecording(), true); }); + it('should prevent recursive calls', async () => { + keyboardMacro.startRecording(); + await keyboardMacro.wrap({ + command: 'kb-macro.wrap', + args: { command: 'internal:log' } + }); + keyboardMacro.finishRecording(); + + assert.deepStrictEqual(logs, []); + assert.deepStrictEqual(keyboardMacro.getCurrentSequence(), []); + }); }); }); From 3cd5be5b7368012269c0cf7bdd4a383a492b076b Mon Sep 17 00:00:00 2001 From: tshino Date: Sat, 26 Feb 2022 00:12:20 +0900 Subject: [PATCH 2/3] Update CHANGELOG --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b542abe..a6589a66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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) From dc050398f4d7c616ce628140c23b86535ae42999 Mon Sep 17 00:00:00 2001 From: tshino Date: Sun, 27 Feb 2022 16:53:45 +0900 Subject: [PATCH 3/3] Make wrap not return Promise to avoid deadlock --- src/keyboard_macro.js | 9 +++- test/suite/keyboard_macro.test.js | 60 ++++++++++++++-------- test/suite/playback_async_commands.test.js | 2 +- test/suite/playback_cursor.test.js | 2 +- test/suite/playback_edit.test.js | 2 +- test/suite/playback_repeat.test.js | 2 +- test/suite/playback_typing.test.js | 8 +-- 7 files changed, 55 insertions(+), 30 deletions(-) diff --git a/src/keyboard_macro.js b/src/keyboard_macro.js index 2007b18a..e0658223 100644 --- a/src/keyboard_macro.js +++ b/src/keyboard_macro.js @@ -249,7 +249,7 @@ 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) { @@ -274,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, @@ -292,6 +298,7 @@ const KeyboardMacro = function({ awaitController }) { abortPlayback, repeatPlayback, repeatPlaybackTillEndOfFile, + wrapSync, wrap, // testing purpose only diff --git a/test/suite/keyboard_macro.test.js b/test/suite/keyboard_macro.test.js index cfd35755..ceb1792a 100644 --- a/test/suite/keyboard_macro.test.js +++ b/test/suite/keyboard_macro.test.js @@ -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' ]); @@ -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' ]); @@ -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, []); @@ -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' ]); @@ -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(); @@ -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(); @@ -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(); @@ -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; @@ -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; @@ -749,9 +749,9 @@ describe('KeybaordMacro', () => { ]); assert.strictEqual(keyboardMacro.isRecording(), true); }); - it('should prevent recursive calls', async () => { + it('should prevent direct recursive calls', async () => { keyboardMacro.startRecording(); - await keyboardMacro.wrap({ + await keyboardMacro.wrapSync({ command: 'kb-macro.wrap', args: { command: 'internal:log' } }); @@ -760,5 +760,23 @@ describe('KeybaordMacro', () => { 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' } + ]); + }); }); }); diff --git a/test/suite/playback_async_commands.test.js b/test/suite/playback_async_commands.test.js index a1148013..ae9bfe9f 100644 --- a/test/suite/playback_async_commands.test.js +++ b/test/suite/playback_async_commands.test.js @@ -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(); }; diff --git a/test/suite/playback_cursor.test.js b/test/suite/playback_cursor.test.js index d9c7a9e9..00a3db97 100644 --- a/test/suite/playback_cursor.test.js +++ b/test/suite/playback_cursor.test.js @@ -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(); }; diff --git a/test/suite/playback_edit.test.js b/test/suite/playback_edit.test.js index 268edeee..1432973f 100644 --- a/test/suite/playback_edit.test.js +++ b/test/suite/playback_edit.test.js @@ -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(); }; diff --git a/test/suite/playback_repeat.test.js b/test/suite/playback_repeat.test.js index 49cc37f3..0c4cb2cf 100644 --- a/test/suite/playback_repeat.test.js +++ b/test/suite/playback_repeat.test.js @@ -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(); }; diff --git a/test/suite/playback_typing.test.js b/test/suite/playback_typing.test.js index 10345504..82c6ce19 100644 --- a/test/suite/playback_typing.test.js +++ b/test/suite/playback_typing.test.js @@ -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'); @@ -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, ' '); @@ -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'); @@ -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();