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
Fixed race condition #2547
base: develop
Are you sure you want to change the base?
Fixed race condition #2547
Conversation
Fixed a race condition when loading map for the first time.
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.
Race conditions can occur because of load order (which script was transferred the fastest), but that's what defer
was supposed to prevent by giving the scripts an explicitly different load order. So I looked around a bit for more specific information.
We assumed defer
would queue execution of that script after non-deferred scripts. Unfortunately that's not the case, defer
scripts are run before the DOMContentLoaded
event is fired.
The body
's onLoad
event, however, fires only when a page is fully loaded. This causes the inconsistent execution order.
And I believe your example of disabling clustering isn't a good one, since custom.js.example
includes code that checks whether the library was already loaded or not. If it was, it calls the existing instance directly.
I've included 2 possible fixes in my review comment. 👇
// wrapper function for real initMap (_initMap) to | ||
// avoid race conditions with custom.js | ||
function initMap() { // eslint-disable-line no-unused-vars | ||
setTimeout(_initMap, 100) |
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.
Setting a 100ms timeout is a guess for execution order (an arbitrary delay). Instead, you have several options that guarantee execution order without delay:
- Replace jQuery's onLoad from custom.js with a self-executing function.
- Define a
customJsLoaded
(not the best naming) incustom.js
and check for its existence inmap.js
. If it exists, call it before doing any initialization so it can properly set all defaults.
@@ -519,7 +519,7 @@ <h3>Location Icon Marker</h3> | |||
</script> | |||
<script src="{{ url_for('static', filename='dist/js/app.min.js').lstrip('/') }}"></script> | |||
<script src="{{ url_for('static', filename='dist/js/map.common.min.js').lstrip('/') }}"></script> | |||
<!-- Load custom JS before map.js so it can change Store defaults. --> | |||
<!-- Custom JS is loaded but not actually executed prior to map.js as all its code is executed in jQuerys onload function. --> |
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.
Read my other comment, and put this message back as it was.
@farstarss Are you still interested in updating this PR? |
Fixed a race condition when loading map for the first time.
Description
Although custom.js is listed above map.js in map.html it's not being executed prior to map.js since the settings inside custom.js are set on page load - which is fine as they depend on Store which is defined in map.js.
The issue is that the google maps script is also going to be executed on page load (=>
defer
attribute) which results in a race condition between custom.js and google maps script callinginitMap()
and sometimes results in custom settings being ignored and default settings being used.To reproduce this just disable clustering in custom.js and load the map in a private tab: You'll see that clustering sometimes happens and sometimes it doesn't.
Motivation and Context
Race conditions should always be addressed and fixed.
How Has This Been Tested?
Locally.
Screenshots (if appropriate):
Types of changes
Checklist: