Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stale data/race condition in feedback mode custom target handling #839

Open
luxaritas opened this issue Dec 3, 2024 · 0 comments
Open
Labels
priority: p3/standard Enhancement with nominal value or bug with nominal impact size: md type: bug Something isn't working

Comments

@luxaritas
Copy link
Member

The code in question:

this._undoBlocks = [];
for (let ii = 0; ii < this._secstructs.length; ii++) {
const datablock: UndoBlock = new UndoBlock(this._sequence, Vienna.NAME);
datablock.setPairs(this._secstructs[ii]);
datablock.setBasics();
this._undoBlocks.push(datablock);
this._poses[ii].targetPairs = this._secstructs[ii];
}
for (const poseField of this._poseFields) {
poseField.updateDeltaEnergyGui();
}
// HACK: This seems to be the most straightforward way to get the saved target structure to display,
// but there's a lot of things to dislike here - the fact we're no longer acting synchronously,
// the potential for race conditions, the code duplication...
solution.queryFoldData().then((fd) => {
if (fd) {
for (let i = 0; i < fd.length; i++) {
const ublk = new UndoBlock(this._sequence, Vienna.NAME);
ublk.fromJSON(fd[i]);
this._secstructs[i] = ublk.targetPairs;
}
} else {
for (let i = 0; i < this._secstructs.length; i++) {
this._secstructs[i] = this._puzzleSecstructs[i];
}
}
this.changeTarget(this._curTargetIndex);
});

Per the comment, I already knew this wasn't great. Something new that was recently reported though is that there are invalid pairing warnings (from Vienna) that can be sent to the console because of this. Why? When switching between solutions, we initially create the UndoBlock with whatever the target structure was from the previous solution, and then update it once we've fetched the updated cached fold data. However, if there are two solutions with different target structures due to custom target structure functionality, there's a reasonable chance that the sequence from one wont "fit" the target of the other, leading to Vienna complaining when we go to compute the energies in setBasics.

It's really not ideal that we update the UI in two steps anyways (where the structure is momentarily wrong), or that there's a potential race condition if you're flipping through solutions and the target structure responses come back out of order, leaving the UI to display the wrong target for the wrong solution.

What we should presumably do here is async-ify these codepaths, and add a UILock that prevents any extra behavior (eg, solution switching) between the start and end of updating the solution. This way we can fetch the fold data before constructing the undo block, and avoid having multiple in-flight requests that can cause a race condition.

@luxaritas luxaritas added type: bug Something isn't working priority: p3/standard Enhancement with nominal value or bug with nominal impact size: md labels Dec 3, 2024
@luxaritas luxaritas added this to Roadmap Dec 3, 2024
@github-project-automation github-project-automation bot moved this to Todo in Roadmap Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p3/standard Enhancement with nominal value or bug with nominal impact size: md type: bug Something isn't working
Projects
Status: Todo
Development

No branches or pull requests

1 participant