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

status-page: fix race condition in initialization. #94

Merged
merged 1 commit into from Jan 8, 2020

Conversation

turboMaCk
Copy link
Member

@turboMaCk turboMaCk commented Jan 7, 2020

Due to the fact that loading state text is inserted on window onload,
this insertion can potentially happen after all other async
actions are already finished and thus replace the actual data.
This fix makes sure that requests are triggered after onload is done.

cc @grahamc

This is what happens when the async actions are resolved with timing that leads to this issue:

image

Steps to reproduce:

  1. make sure that client side cache is disabled in browser
    • common way is to set "disable caching while developer console is open" and open the console
  2. refresh page several times until you hit critical timing in which issue occures.

@turboMaCk turboMaCk force-pushed the bug/status-fix-race-condition branch from 552b023 to 085287c Compare January 7, 2020 16:47
@grahamc
Copy link
Member

grahamc commented Jan 7, 2020

cc @samueldr mind taking a look-see?

@@ -1,7 +1,12 @@
window.onload = (_) => {
Copy link
Member Author

@turboMaCk turboMaCk Jan 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed this wildcard _ for the argument name because in JS all arguments are optional (with default to undefined) and arrity of functions is pretty much dynamic. While I personally also like this convention of denotating ignored arguments with _ the problem in JS case is that one can never trust the number of arguments so it can be missleading when someone less experienced is reading the code and making assumptions about arity based on those wildcards even though in this case the arity won't change (unless some monkeypatching gets involved).

to make matters worse there are polupar libraries like lodash and underscore that make use of _ as a namespace for functions so using _ as an argument name can cause some unfortunete hard to track shadowing of those namespaces.

Not a big deal but better safe than sorry I would say.

@turboMaCk turboMaCk force-pushed the bug/status-fix-race-condition branch from 085287c to b90b5cb Compare January 7, 2020 17:06
@turboMaCk
Copy link
Member Author

turboMaCk commented Jan 7, 2020

indentation of init changed to 2 space to be consistent with rest of the code. This is ready for merge from my side.

Due to the fact that loading state text is inserted on window onload,
this insertion can potentially happen **after** all other async
actions are already finished and thus replace the actual data.
This fix makes sure that requests are triggered after onload is done.
@turboMaCk turboMaCk force-pushed the bug/status-fix-race-condition branch from b90b5cb to 69bfb8c Compare January 7, 2020 17:08
@zimbatm zimbatm merged commit ae96158 into NixOS:master Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants