-
Notifications
You must be signed in to change notification settings - Fork 303
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
Fix infinite reload of sample lists #4641
Conversation
@@ -1004,14 +1007,6 @@ export class QueryStore { | |||
); | |||
}, | |||
default: [], | |||
onResult: () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BasLee this is the offending onResult. It depends upon this._selectedSampleListId, so when it mutates that value it re-fires invoke and is circular.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that it was necessary in order to clear the selection when user selected a different study. This can/should be handled on read by checking to see if the _selectedSampleListId matches selectedStudies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the weird thing was, that the infinite loop even existed without any code in the onResult: it seemed like the presence of onResult itself was causing the loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting. i will test that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see that you are right. in my testing, the problem is that mobxpromise's onResult references this.result in order to pass it to the callback:
when i use result instead of this.result, the problem goes away. will have to make this change to the library.
nevertheless, i think getting rid of onResult in general is a good idea as it is almost never actually necessary.
} else { | ||
this.singlePageAppSubmitRoutine(urlParams.query, currentTab); | ||
} | ||
this.singlePageAppSubmitRoutine(urlParams.query, currentTab); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is an unrelated improvement/bug fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, download mode doesn't exist anymore so I've been trying to clean this stuff out, but i should probably do it in a seperate PR
@BasLee thanks to your detective work, we have found that there is a problem in mobxpromise library itself. However, that doesn't change the fact that the use of onResult in this case (and probably almost all other cases) is misguided. In general, it's subject to observability problems. I.e. I think that my refactor makes sense to merge and we can follow up later with the change to library. |
When a user views a virtual study without any mutations, the result view page will fire an infinite number of backend requests to
/api/session/virtual_study
.This is caused by an infinite loop in the
QueryStore
that keeps recalculatingsampleLists
when selecting a virtual study.(This PR also adds some documentation on the
localdev
property that is needed to debug the frontend using https.)Reproduce bug
The bug can be reproduced by:
Fix
The onResult handlers were mutating state on which the invoke dependend, causing infinite loop. This gets rid of the onResult and moves the clearing of the sample list selection state to the study selection handler. Whenever studies are selected/deselected, the sample list is reset to default.