🐛Fix variable service race.#22375
Conversation
|
cc/ @mattwomple I want to make sure you have a change to test this time. |
|
Thanks @calebcordry. Sorry I couldn't review over the weekend, but I expect to be able to complete a review before 12:00 PM PDT if that works for you. If not, I'll still leave a review after merge. Edit: Ah, my phone is logged into my personal GitHub account. This is @mattwomple. |
|
@mattwomple no worries, I wasn't expecting you to work on weekend. 12pm is fine, I would just like to get this in sometime today. |
| * @param {!Element|!ShadowRoot|!../../../src/service/ampdoc-impl.AmpDoc} elementOrAmpDoc | ||
| * @return {!Promise<!VariableService>} | ||
| */ | ||
| export function variableServicePromiseForDoc(elementOrAmpDoc) { |
There was a problem hiding this comment.
do we still need the other sync method?
There was a problem hiding this comment.
The other classes created by amp-analytics (linker, cookie-writer, requests etc) are still using the sync method. This is ok because they get created after amp-analytics resolves the promise. I thought about changing them all but I think it would make the code harder to follow in these other files.
|
@calebcordry This commit appears to resolve the issues I've seen with moving amp-analytics variables to the document-level. I largely tested this from a functionality and regression standpoint with the following page that incorporates amp-bind and amp-analytics in a PWA: bind-analytics-pwa.html (and associated AMP pages -amp1.html and -amp2.html). These files can be dropped into the LGTM |
|
Not sure why the tests are stalled. Rebasing to re-trigger. |
|
I found the the comment for a similar case amphtml/extensions/amp-analytics/0.1/instrumentation.js Lines 127 to 130 in 595ba49 Looks like the immediate fix is to asynchronously resolve doc level services that are registered upon element registration. If that's the case should we apply the same to amphtml/extensions/amp-analytics/0.1/amp-analytics.js Lines 781 to 792 in 595ba49 What about other components? Do you think this is a race condition that should be fixed by the shadow doc code instead? It would be great if you could point me to where services are registered for individual doc. Thank you. |
The only getter I see for the activity service is here https://github.com/ampproject/amphtml/blob/master/src/services.js#L120 and it looks like its already returning a promise. Let me know if you see another that is potentially problematic.
Maybe, I was chatting with @jridgewell earlier today about this, and I think he is looking into it. @jridgewell could you provide @zhouyx with a code pointer here? |
|
@zhouyx merging this to make canary cut. Let me know if you want me to open an issue around shadow behavior and we can continue discussion there. |
|
That'd be great. Thanks. My concern is that I expect it's common to encounter this race condition and there could be existing bugs with other components already. |
Introduced another bug in #22300. When loading new amp docs into shadow shell shell service registration is asynchronous, but I was trying to retrieve the service synchronously. This changes amp-analytics to wait for a promise of this service.
All other analytics files can continue to use the sync method as their creating analytics element will have waited for promise to resolve.