Bug report: Race condition when the url contains input encoding that sometimes makes UI tests fail
zb3 opened this issue · 5 comments
(I'm not sure about this analysis and how that should be fixed hence I submit this as an issue not a PR)
In the loadUriParams
function we see the following code:
// Input Character Encoding
// Must be set before the input is loaded
if (this.uriParams.ienc) {
this.manager.input.chrEncChange(parseInt(this.uriParams.ienc, 10), true);
}
The chrEncChange
function in turn calls inputChange
:
chrEncChange(chrEncVal, manual=false) {
if (typeof chrEncVal !== "number") return;
this.inputChrEnc = chrEncVal;
this.encodingState = manual ? 2 : this.encodingState;
this.inputChange();
}
Note that at the time this is called, the input is empty.. (edit: or not changed)
Then inside inputChange
we have a debounce call which actually works like setTimeout. Since the doc is empty, this function will be called after 20ms and will call updateInputValue
with the current value of the input:
const inputLength = this.inputEditorView.state.doc.length;
let delay;
if (inputLength < 10000) delay = 20;
else if (inputLength < 100000) delay = 50;
else if (inputLength < 1000000) delay = 200;
else delay = 500;
// ...
debounce(function(e) {
// ...
const value = this.getInput();
this.updateInputValue(activeTab, value);
And here's the problem... by the time this is called, the input might not yet be ready, hence we overwrite the input value with the previous one / empty string.
This makes the "Loading from URL" test fail on my machine, although not always. It's easier to reproduce if the "delay" is artificially lowered to 1.
Would it make sense to create a separate parameter for chrEncChange whether to skip calling inputChange?
This goes even further..
I see the "Loading from URL" test sometimes fails also because the setInput
method called when loading URI params in the App
class uses silent: true
, but.. since the fileLoaded
method calls it with silent: false
, the debounce
will cause the output to be overwriten with the old file contents.
Additionally, I see that the set
method in the InputWaiter
class dispatches a statechange
even if it's not a silent update.. but to me it seems redundant because that even will be dispatched by inputChange
, so this can cause additional autobake..
I was able to get rid of the error by making setInput
in the App
class do a non-silent update, but in the loadURIParams
method I made the dispatch of the "statechange" even conditional - if setInput
wasn't called.
I think there are more race conditions, for example when a previous bake takes longer than the new one and overwrites the output content with stale output..
Also since the debounce
was introduced for updates, the autoBakePause
variable inside loadURIParams
no longer works, as the timeout is queued and by the time it executes autoBakePause
is already false
Thanks very much for your work on diagnosing this. I've made a lot of changes to the execution flow and triggers over the last year and I'm not at all surprised that there are some race conditions caused by the added complexity. Thank you for spotting some of them!
I've removed the concept of autoBakePause
altogether as you are correct that the use of debounce()
makes it redundant.
I've also removed the statechange
triggered by the InputWaiter.set()
function. You're right that this should always be triggered by inputChange()
anyway. Extensive tests seem to suggest that removing it hasn't broken anything and I can't think of any scenarios where it would be required now. It must just be a remnant of a previous execution flow.
I have reverted the changes you made to the App.inputChange()
call where you set the silent
flag to false
and then added a condition in the loadURIParams
function. This doesn't feel like the right way to solve the problem as the silent
flag is designed specifically to handle this case. I'm not quite sure I understand the scenario where it breaks. Under what circumstances would the fileLoaded
method trigger at the same time as the loadURIParams
method? Files can't be loaded from the URI, so is this ever a problem? I could easily be wrong here, but if I am, could someone explain a way to reproduce this problem so we can work out the right way to solve it?
Thanks once again for digging into this - it's non-trivial and you explained it well.
Under what circumstances would the fileLoaded method trigger at the same time as the loadURIParams method?
@n1474335 The problem is that fileLoaded
will cause a method to be called 20ms after it is invoked.. that method will.. umm "cement" the input that is currently in the area.. so if loadURIParams uses silent, it won't queue its "countermeasure" so the old file can overwrite the content.. of course this description doesn't make much sense (I describe what happens, the "why" is unclear to me too) but I remember you could reproduce it by running two UI tests in order - first the file test and then the load uri test. The second test failed because contents were overwritten with the file contents..
I've unfortunately introduced another UI test failure with the cancel on autobake PR, and I wasn't able to reproduce it at all.. it only happens when the tests are run.
IOW, silent equal to false inhibits emitting statechange after 20ms, so it's not equal to inhibiting emitting statechange now