From f48bb375b8d22487e395f182551abf9aaa3faa27 Mon Sep 17 00:00:00 2001 From: tshino Date: Mon, 10 Jan 2022 23:50:30 +0900 Subject: [PATCH 1/4] Add makeQueueableCommand() --- src/reentrant_guard.js | 55 ++++++- test/suite/reentrant_guard.test.js | 254 +++++++++++++++++++++++++++++ 2 files changed, 301 insertions(+), 8 deletions(-) diff --git a/src/reentrant_guard.js b/src/reentrant_guard.js index f54bcd06..70a8aea5 100644 --- a/src/reentrant_guard.js +++ b/src/reentrant_guard.js @@ -2,7 +2,11 @@ const reentrantGuard = (function() { - let locked = false; + const state = { + locked: false, + queueable: false + }; + const queue = []; let printError = defaultPrintError; function defaultPrintError(error) { @@ -17,31 +21,64 @@ const reentrantGuard = (function() { const makeGuardedCommand = function(body) { return async function(args) { - if (locked) { + if (state.locked) { return; } - locked = true; + state.locked = true; try { await body(args); } catch (error) { printError(error); } finally { - locked = false; + state.locked = false; } }; }; const makeGuardedCommandSync = function(func) { return function(args) { - if (locked) { + if (state.locked) { return; } - locked = true; + state.locked = true; try { func(args); } catch (error) { printError(error); } finally { - locked = false; + state.locked = false; + } + }; + }; + const makeQueueableCommand = function(body, { queueSize = 0 } = {}) { + return async function(args) { + if (state.locked) { + if (state.queueable) { + if (!queueSize || queue.length < queueSize) { + queue.push([body, args]); + } + } + return; + } + state.locked = true; + state.queueable = true; + try { + try { + await body(args); + } catch (error) { + printError(error); + } + while (0 < queue.length) { + try { + const [body, args] = queue[0]; + queue.splice(0, 1); + await body(args); + } catch (error) { + printError(error); + } + } + } finally { + state.locked = false; + state.queueable = false; } }; }; @@ -49,9 +86,11 @@ const reentrantGuard = (function() { return { makeGuardedCommand, makeGuardedCommandSync, + makeQueueableCommand, // testing purpose only - setPrintError + setPrintError, + getQueueLength: function() { return queue.length; } }; })(); diff --git a/test/suite/reentrant_guard.test.js b/test/suite/reentrant_guard.test.js index 0f220d76..c2ae7ede 100644 --- a/test/suite/reentrant_guard.test.js +++ b/test/suite/reentrant_guard.test.js @@ -252,4 +252,258 @@ describe('reentrantGuard', () => { } }); }); + describe('makeQueueableCommand', () => { + const makeQueueableCommand = reentrantGuard.makeQueueableCommand; + const logs = []; + beforeEach(() => { + logs.length = 0; + }); + it('should return an async function', async () => { + const func = makeQueueableCommand(() => {}); + assert.strictEqual(typeof func, 'function'); + assert.strictEqual(func.constructor.name, 'AsyncFunction'); + }); + it('should return a function that invokes the given function', async () => { + const target = function() { + logs.push('hello'); + }; + const func = makeQueueableCommand(target); + assert.deepStrictEqual(logs, []); + await func(); + assert.deepStrictEqual(logs, [ 'hello' ]); + await func(); + assert.deepStrictEqual(logs, [ 'hello', 'hello' ]); + }); + it('should return a function that passes arguments to the given function', async () => { + const target = function(arg) { + logs.push('hello ' + arg); + }; + const func = makeQueueableCommand(target); + await func('world'); + assert.deepStrictEqual(logs, [ 'hello world' ]); + }); + it('should return a function that invokes the given async function', async () => { + const asyncTarget = async function() { + logs.push('hello'); + await TestUtil.sleep(10); + logs.push('bye'); + }; + const func = makeQueueableCommand(asyncTarget); + logs.push('before call'); + await func(); + logs.push('after call'); + assert.deepStrictEqual(logs, [ + 'before call', + 'hello', + 'bye', + 'after call' + ]); + }); + it('should enqueue and serialize invocation of the function', async () => { + const asyncTarget = async function(arg) { + logs.push('hello ' + arg); + await TestUtil.sleep(10); + logs.push('bye'); + }; + const func = makeQueueableCommand(asyncTarget); + logs.push('before concurrent call'); + const promise1 = func('1'); + const promise2 = func('2'); + assert.strictEqual(reentrantGuard.getQueueLength(), 1); + await Promise.all([promise1, promise2]); + logs.push('after concurrent call'); + assert.strictEqual(reentrantGuard.getQueueLength(), 0); + await func('3'); + logs.push('after third call'); + assert.strictEqual(reentrantGuard.getQueueLength(), 0); + assert.deepStrictEqual(logs, [ + 'before concurrent call', + 'hello 1', + 'bye', + 'hello 2', + 'bye', + 'after concurrent call', + 'hello 3', + 'bye', + 'after third call' + ]); + }); + it('should not enqueue over queueSize if specified', async () => { + const asyncTarget = async function(arg) { + logs.push('hello ' + arg); + await TestUtil.sleep(10); + logs.push('bye'); + }; + const func = makeQueueableCommand(asyncTarget, { queueSize: 3 }); + logs.push('before concurrent call'); + const promise1 = func('1'); + const promise2 = func('2'); + const promise3 = func('3'); + const promise4 = func('4'); + const promise5 = func('5'); + assert.strictEqual(reentrantGuard.getQueueLength(), 3); + await Promise.all([promise1, promise2, promise3, promise4, promise5]); + logs.push('after concurrent call'); + await func('6'); + logs.push('after last call'); + assert.strictEqual(reentrantGuard.getQueueLength(), 0); + assert.deepStrictEqual(logs, [ + 'before concurrent call', + 'hello 1', + 'bye', + 'hello 2', + 'bye', + 'hello 3', + 'bye', + 'hello 4', + 'bye', + 'after concurrent call', + 'hello 6', + 'bye', + 'after last call' + ]); + }); + it('should prevent queueable function from interrupting guarded functions', async () => { + const asyncTarget = async function(arg) { + logs.push('hello ' + arg); + await TestUtil.sleep(10); + logs.push('bye'); + }; + const guarded = reentrantGuard.makeGuardedCommand(asyncTarget); + const queueable = makeQueueableCommand(asyncTarget); + logs.push('before concurrent call'); + const promise1 = guarded('guarded'); + const promise2 = queueable('queueable'); + assert.strictEqual(reentrantGuard.getQueueLength(), 0); + await Promise.all([promise1, promise2]); + logs.push('after concurrent call'); + assert.strictEqual(reentrantGuard.getQueueLength(), 0); + assert.deepStrictEqual(logs, [ + 'before concurrent call', + 'hello guarded', + 'bye', + 'after concurrent call' + ]); + }); + it('should prevent guarded function from interrupting queueable functions', async () => { + const asyncTarget = async function(arg) { + logs.push('hello ' + arg); + await TestUtil.sleep(10); + logs.push('bye'); + }; + const guarded = reentrantGuard.makeGuardedCommand(asyncTarget); + const queueable = makeQueueableCommand(asyncTarget); + logs.push('before concurrent call'); + const promise1 = queueable('queueable'); + const promise2 = guarded('guarded'); + assert.strictEqual(reentrantGuard.getQueueLength(), 0); + await Promise.all([promise1, promise2]); + logs.push('after concurrent call'); + assert.strictEqual(reentrantGuard.getQueueLength(), 0); + assert.deepStrictEqual(logs, [ + 'before concurrent call', + 'hello queueable', + 'bye', + 'after concurrent call' + ]); + }); + it('should prevent the function from leaking exceptions', async () => { + const old = reentrantGuard.setPrintError(error => { + logs.push('error: ' + error); + }); + try { + const willThrow = makeQueueableCommand(function() { + logs.push('will throw'); + throw 'error'; + }); + const wontThrow = makeQueueableCommand(function() { + logs.push('will not throw'); + }); + logs.push('before call'); + await willThrow(); + await wontThrow(); + logs.push('after call'); + assert.deepStrictEqual(logs, [ + 'before call', + 'will throw', + 'error: error', + 'will not throw', + 'after call' + ]); + } finally { + reentrantGuard.setPrintError(old); + } + }); + it('should invoke queued function even if another queued function threw exception (1)', async () => { + const old = reentrantGuard.setPrintError(error => { + logs.push('error: ' + error); + }); + try { + const willThrow = makeQueueableCommand(async function() { + logs.push('will throw...'); + await TestUtil.sleep(10); + logs.push('will throw now'); + throw 'error'; + }); + const wontThrow = makeQueueableCommand(async function() { + logs.push('will not throw'); + }); + logs.push('before concurrent call'); + const promise1 = willThrow(); + const promise2 = wontThrow(); + await Promise.all([promise1, promise2]); + logs.push('after concurrent call'); + assert.deepStrictEqual(logs, [ + 'before concurrent call', + 'will throw...', + 'will throw now', + 'error: error', + 'will not throw', + 'after concurrent call' + ]); + } finally { + reentrantGuard.setPrintError(old); + } + }) + it('should invoke queued function even if another queued function threw exception (2)', async () => { + const old = reentrantGuard.setPrintError(error => { + logs.push('error: ' + error); + }); + try { + const wontThrow1 = makeQueueableCommand(async function() { + logs.push('will not throw (1) begin'); + await TestUtil.sleep(10); + logs.push('will not throw (1) end'); + }); + const willThrow = makeQueueableCommand(async function() { + logs.push('will throw...'); + await TestUtil.sleep(10); + logs.push('will throw now'); + throw 'error'; + }); + const wontThrow2 = makeQueueableCommand(async function() { + logs.push('will not throw (2)'); + }); + logs.push('before concurrent call'); + const promise1 = wontThrow1(); + const promise2 = willThrow(); + await promise1; + const promise3 = wontThrow2(); + await Promise.all([promise2, promise3]); + logs.push('after concurrent call'); + assert.deepStrictEqual(logs, [ + 'before concurrent call', + 'will not throw (1) begin', + 'will not throw (1) end', + 'will throw...', + 'will throw now', + 'error: error', + 'will not throw (2)', + 'after concurrent call' + ]); + } finally { + reentrantGuard.setPrintError(old); + } + }) + }); }); From a380f80262e41202d77f7c047a248fcc1cac68ca Mon Sep 17 00:00:00 2001 From: tshino Date: Tue, 11 Jan 2022 20:51:55 +0900 Subject: [PATCH 2/4] Make 'wrap' command queueable by makeQueueableCommand() --- src/keyboard_macro.js | 13 ++++++--- test/suite/keyboard_macro.test.js | 44 ++++++++++++++++++++++++++++--- 2 files changed, 50 insertions(+), 7 deletions(-) diff --git a/src/keyboard_macro.js b/src/keyboard_macro.js index 2ff7904d..ae328064 100644 --- a/src/keyboard_macro.js +++ b/src/keyboard_macro.js @@ -184,7 +184,13 @@ const KeyboardMacro = function({ awaitController }) { return spec; }; - const wrap = reentrantGuard.makeGuardedCommand(async function(args) { + // WrapQueueSize + // independently adjustable value. + // min value is 1. + // greater value reduces input rejection. 2 or 3 is enough. + // greater value leads to too many queued and delayed command execution. + const WrapQueueSize = 2; + const wrap = reentrantGuard.makeQueueableCommand(async function(args) { if (recording) { const spec = makeCommandSpec(args); if (!spec) { @@ -204,7 +210,7 @@ const KeyboardMacro = function({ awaitController }) { } } } - }); + }, { queueSize: WrapQueueSize }); return { RecordingStateReason, @@ -228,7 +234,8 @@ const KeyboardMacro = function({ awaitController }) { isRecording: () => { return recording; }, isPlaying: () => { return playing; }, getCurrentSequence: () => { return sequence.get(); }, - setShowInputBox // testing purpose only + setShowInputBox, + WrapQueueSize }; }; diff --git a/test/suite/keyboard_macro.test.js b/test/suite/keyboard_macro.test.js index eac74b97..82d4338c 100644 --- a/test/suite/keyboard_macro.test.js +++ b/test/suite/keyboard_macro.test.js @@ -551,19 +551,55 @@ describe('KeybaordMacro', () => { { command: 'internal:log', args: { test: '1' } } ]); }); - it('should prevent reentry', async () => { + it('should enqueue and serialize concurrent call', async () => { keyboardMacro.startRecording(); const promise1 = keyboardMacro.wrap({ command: 'internal:log' }); const promise2 = keyboardMacro.wrap({ command: 'internal:log' }); await Promise.all([promise1, promise2]); keyboardMacro.finishRecording(); - assert.deepStrictEqual(logs, [ 'begin', 'end' ]); + assert.deepStrictEqual(logs, [ + 'begin', 'end', + 'begin', 'end' + ]); assert.deepStrictEqual(keyboardMacro.getCurrentSequence(), [ + { command: 'internal:log' }, { command: 'internal:log' } ]); }); - it('should prevent other commands to preempt (cancelRecording)', async () => { + 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) + for (let i = 0; i < keyboardMacro.WrapQueueSize; i++) { + promises.push(keyboardMacro.wrap({ command: 'internal:log' })); // (2) to (WrapQueueSize + 1) + } + await Promise.all(promises); + await keyboardMacro.wrap({ command: 'internal:log' }); // (WrapQueueSize + 2) + keyboardMacro.finishRecording(); + + const expectedLog = Array(keyboardMacro.WrapQueueSize + 2).fill(['begin', 'end']).flat(); + const expectedSequence = Array(keyboardMacro.WrapQueueSize + 2).fill({ command: 'internal:log' }); + assert.deepStrictEqual(logs, expectedLog); + assert.deepStrictEqual(keyboardMacro.getCurrentSequence(), expectedSequence); + }); + it('should overflow when over WrapQueueSize concurrent calls made', async () => { + keyboardMacro.startRecording(); + const promises = []; + promises.push(keyboardMacro.wrap({ 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) + } + await Promise.all(promises); + await keyboardMacro.wrap({ command: 'internal:log' }); // (WrapQueueSize + 3) + keyboardMacro.finishRecording(); + + const expectedLog = Array(keyboardMacro.WrapQueueSize + 2).fill(['begin', 'end']).flat(); + const expectedSequence = Array(keyboardMacro.WrapQueueSize + 2).fill({ command: 'internal:log' }); + assert.deepStrictEqual(logs, expectedLog); + assert.deepStrictEqual(keyboardMacro.getCurrentSequence(), expectedSequence); + }); + it('should prevent other commands from interrupting wrap command (cancelRecording)', async () => { keyboardMacro.startRecording(); const promise1 = keyboardMacro.wrap({ command: 'internal:log' }); keyboardMacro.cancelRecording(); // <-- @@ -575,7 +611,7 @@ describe('KeybaordMacro', () => { ]); assert.strictEqual(keyboardMacro.isRecording(), true); }); - it('should prevent other commands to preempt (finishRecording)', async () => { + it('should prevent other commands from interrupting wrap command (finishRecording)', async () => { keyboardMacro.startRecording(); const promise1 = keyboardMacro.wrap({ command: 'internal:log' }); keyboardMacro.finishRecording(); // <-- From 2acab0cf81ffa218369c34c8d8b9eca6dab22982 Mon Sep 17 00:00:00 2001 From: tshino Date: Tue, 11 Jan 2022 21:36:27 +0900 Subject: [PATCH 3/4] Update CHANGELOG --- CHANGELOG.md | 2 ++ src/keyboard_macro.js | 1 + 2 files changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a227ccb..f5d30edc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,8 @@ All notable changes to the Keyboard Macro Bata extension will be documented in this file. ### [Unreleased] +- Feature + - Made the `kb-macro.wrap` command queueable to reduce input misses during recording. [#32](https://github.com/tshino/vscode-kb-macro/pull/32) - Documentation - Added 'Tips' section to the README. diff --git a/src/keyboard_macro.js b/src/keyboard_macro.js index ae328064..7a722631 100644 --- a/src/keyboard_macro.js +++ b/src/keyboard_macro.js @@ -189,6 +189,7 @@ const KeyboardMacro = function({ awaitController }) { // min value is 1. // greater value reduces input rejection. 2 or 3 is enough. // 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) { if (recording) { From 647d8bd78c9d7cec80f22cd5c697f582c8de1cf1 Mon Sep 17 00:00:00 2001 From: tshino Date: Tue, 11 Jan 2022 21:40:16 +0900 Subject: [PATCH 4/4] Small refactoring --- test/suite/keyboard_macro.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/suite/keyboard_macro.test.js b/test/suite/keyboard_macro.test.js index 82d4338c..35a00154 100644 --- a/test/suite/keyboard_macro.test.js +++ b/test/suite/keyboard_macro.test.js @@ -551,7 +551,7 @@ describe('KeybaordMacro', () => { { command: 'internal:log', args: { test: '1' } } ]); }); - it('should enqueue and serialize concurrent call', async () => { + it('should enqueue and serialize concurrent calls', async () => { keyboardMacro.startRecording(); const promise1 = keyboardMacro.wrap({ command: 'internal:log' }); const promise2 = keyboardMacro.wrap({ command: 'internal:log' });