Closed abirmingham closed 7 months ago
Another alternative - keep the lock, but get rid of the queue. So if a command is received while another is in flight the command could be ignored or an error could be shown. My only concern about showing an error is that vscode makes it very easy to issue multiple commands in the same second (for example, my repro of this issue is tapping control twice on a MM pointing to a script that I don't have locally).
It looks like there isn't a memory issue with endlessly chaining a promise, so I've removed the task counter as well as the nulling out of the blocking task.
Tested memory using the following on nodejs v12.22.9:
const createOneMegabyteString = () => {
// Each character is assumed to be 2 bytes in UTF-16
// 1 MB = 1,048,576 bytes, so we need half as many characters
const size = 1024 * 1024 / 2; // Number of characters for 1 MB
const character = 'a'; // The character to repeat
return character.repeat(size);
}
// Increases memory a lot:
global.data = []
for (let i = 0; i < 1000000; i++) {
global.data.push(createOneMegabyteString())
}
// Does not increase memory:
global.foo = Promise.resolve()
for (let i = 0; i < 100; i++) {
global.foo = global.foo.then(() => {
const localData = []
for (let j = 1000000; j < 10; j++) {
localData.push(createOneMegabyteString())
}
})
}
I just found a bug with this. The promise will not resolve if an exception is thrown. Working on a solution now.
Should be fixed now.
Closing for now. Will rebase/reopen after #39 is accepted
Notes for myself for later:
I left this change off #40 because it's not as pressing and I'd like to pursue an option B for this change. Specifically ensureGameConnection()
would return a client that has a lock, so that client is never available outside of a locked context. In this case, ensureGameConnection()
would always be called at the top level, and client
would be passed to consumers in the stack, e.g. commandCheckDate()
would call ensureGameConnection()
and pass client
to GSLExtension.checkModifiedDate()
.
This fixes a race condition where
definitionProvider
can initiate multiple commands to the game terminal in parallel. If these commands interleave then incorrect output from the terminal might be read, resulting in a script being overwritten with output such as[/ms 11571]
.Here we run game commands in serial. Alternative solutions considered:
definitionProvider.ts
. This would solve the immediate problem but there may be other callers.Update
ensureGameConnection()
to accept a callback like(client: EditorClient): Promise<void>
. This is probably a better solution than exposingclient.lock
because it means that callers cannot accessclient
outside of the locked context. However, it's a larger diff and therefore a potentially riskier change.I recommend reading this MR with whitespace diff off: https://github.com/pltrant/GSL-Editor/pull/38/files?diff=split&w=1