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

Fix a race condition when refresh is called closely to defineSlots #206

Merged

Conversation

patrick-mcdougle
Copy link
Contributor

Hello!

This bug was a doozy to find, but I think this is the code required to fix it.

If there was a pending (un-resolved) load promise with defineSlot commands sitting waiting to be added to the gpt command queue and refresh was called, refresh would run too soon, calling refresh before the slots were defined. By linking the refresh to the loadPromise, we guarantee that slots that are sitting in the load queue are defined before refresh is called.

This is my first contribution to this library and there's not a formal contributing instructions so please let me know if there's anything else you need from me.

@patrick-mcdougle
Copy link
Contributor Author

I made some diagrams to attempt to illustrate this, but async is really hard to visualize. Basically all of the defineSlots calls run synchronously after the googletag promise resolves (but queuing things onto the googletag cmd queue).

Before

A call to refresh could enqueue a refresh too soon
beforeChange

After

Refresh is forced to queue behind the most recent call to load
afterChange

@jaanauati
Copy link
Owner

@patrick-mcdougle this makes sense!, i'll try to get this tested and merged by the end of the week. Thank you!

@jaanauati
Copy link
Owner

lgtm

@jaanauati jaanauati merged commit 7de86b7 into jaanauati:master Sep 24, 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.

2 participants