Changed cid-impl to handle a racy condition. #2923
Conversation
| // The initial CID generation is inherently racy. First one that gets | ||
| // consent wins. | ||
| const relookup = getCookie(win, scope); | ||
| if (!relookup) { | ||
| setCidCookie(win, scope, newCookie); | ||
| } | ||
| return Promise.resolve(relookup || newCookie); |
There was a problem hiding this comment.
No need to .resolve, just retuning will do that.
There was a problem hiding this comment.
ack. Letting the tests run to make sure nothing is broken. Will fix it after that.
|
/cc @cramforce |
| @@ -182,15 +182,15 @@ function getOrCreateCookie(cid, getCidStruct, persistenceConsent) { | |||
| const newCookie = 'amp-' + cid.sha384Base64_(getEntropy(win)); | |||
|
|
|||
| // Store it as a cookie based on the persistence consent. | |||
| persistenceConsent.then(() => { | |||
| return persistenceConsent.then(() => { | |||
There was a problem hiding this comment.
This will not work. These promises have a cyclic dep.
The real fix is to use the baseCid_ instance var to short circuit the second entry into this function without reading cookies.
There was a problem hiding this comment.
Can we add a line before
this.baseCid_ = newCookie
and the same for localStorage.
|
ptal. I also added the test cases for the bug. |
| /** | ||
| * Cache to store external cids. Scope is used as the key and cookie value | ||
| * is the value. | ||
| * @private{Object.<string, string>} |
|
LGTM |
| * is the value. | ||
| * @private{Object.<string, string>} | ||
| */ | ||
| this.externalCidCache_ = {}; |
There was a problem hiding this comment.
use Object.create(null) instead
There was a problem hiding this comment.
didn't know about this. Thanks!
When the cid is requested twice in a row and no cookie is present, the cid in the first call is different from the seconed one.
When the cid is requested twice in a row and no cookie is present, the
cid in the first call is different from the seconed one.