Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Fix race condition when sending daily stats (take 2) #29
Conversation
Debugging current tests was a mess since previously created stores kept running and reporting their states. With this commit, and `end()` method has been added to the Store class that allows to stop the interval that reports events. This method can also be used by consumers whenever the store needs to be disposed.
…tats This commits adds a simple locking mechanism to prevent the race conditions that could happen if two different telemetry instances run in parallel.
|
I left one comment to clarify one aspect of the newly-introduced code, but this looks good otherwise! |
| @@ -160,10 +170,26 @@ export class StatsStore { | |||
| if (this.optOut || this.isDevMode) { | |||
| return; | |||
| } | |||
| const stats = await this.getDailyStats(); | |||
|
|
|||
| // If multiple instances of `telemetry` are being run from different | |||
as-cii
Jun 3, 2019
When we discussed this synchronously, I believe you mentioned localStorage would itself implement a mutex around accesses to any of its keys, and that the lock would be released before the next tick of the loop. Are we taking advantage of those semantics here? If so, can we document that as well (maybe linking to some resource describing that behavior)?
When we discussed this synchronously, I believe you mentioned localStorage would itself implement a mutex around accesses to any of its keys, and that the lock would be released before the next tick of the loop. Are we taking advantage of those semantics here? If so, can we document that as well (maybe linking to some resource describing that behavior)?
rafeca
Jun 3, 2019
Author
Contributor
Yes exactly, I've just double-checked more deeply an this seems to be quite complex thing 😅
In the W3C Storage standard there's some information about it.
But then there's this article (quite old imho), which states that localStorage is not thread safe in most browsers.
After researching a bit, seems like at the time (2012) most of the engines didn't implement the mutex (to be fair, most of the browsers didn't run tabs in separate threads), so this could be an issue back then. It even seems that Chrome removed temporarily the mutexes for performance reasons. Today, with the predominance of chromium and with the fact that Atom runs on electron this doesn't seem to be a problem.
Nowadays, chromium (and therefore electron) seems to have a few mechanism to avoid the race conditions but honestly I haven't gone deep into their code to fully understand what's doing.
To be completely sure, I've just tried by brute force to cause a race condition both on Atom and Chrome and was unable to do so. This is the code that I've used:
// window 1
const timer = setInterval(() => {
if (localStorage.getItem('foo') === '1') {
return;
}
console.log('trying to cause a race condition');
let i = 4000000;
while (i--) {
if (localStorage.getItem('foo') === '1') {
console.error('Race condition!');
clearInterval(timer);
break;
}
}
console.log('no race condition yet...');
}, 1000);
// =========================
// window 2
let value = '1';
setInterval(() => {
value = value === '1' ? '0' : '1';
localStorage.setItem('foo', value);
}, 1);
I've run this super abusive code for almost an hour with no race condition happening, so I'm pretty sure that even if there was a possibility of a race condition it won't ever happen in the metrics scenario.
I'm gonna add a comment linking to the standard though 😄
Yes exactly, I've just double-checked more deeply an this seems to be quite complex thing
In the W3C Storage standard there's some information about it.
But then there's this article (quite old imho), which states that localStorage is not thread safe in most browsers.
After researching a bit, seems like at the time (2012) most of the engines didn't implement the mutex (to be fair, most of the browsers didn't run tabs in separate threads), so this could be an issue back then. It even seems that Chrome removed temporarily the mutexes for performance reasons. Today, with the predominance of chromium and with the fact that Atom runs on electron this doesn't seem to be a problem.
Nowadays, chromium (and therefore electron) seems to have a few mechanism to avoid the race conditions but honestly I haven't gone deep into their code to fully understand what's doing.
To be completely sure, I've just tried by brute force to cause a race condition both on Atom and Chrome and was unable to do so. This is the code that I've used:
// window 1
const timer = setInterval(() => {
if (localStorage.getItem('foo') === '1') {
return;
}
console.log('trying to cause a race condition');
let i = 4000000;
while (i--) {
if (localStorage.getItem('foo') === '1') {
console.error('Race condition!');
clearInterval(timer);
break;
}
}
console.log('no race condition yet...');
}, 1000);
// =========================
// window 2
let value = '1';
setInterval(() => {
value = value === '1' ? '0' : '1';
localStorage.setItem('foo', value);
}, 1);I've run this super abusive code for almost an hour with no race condition happening, so I'm pretty sure that even if there was a possibility of a race condition it won't ever happen in the metrics scenario.
I'm gonna add a comment linking to the standard though
Summary
This PR adds a simple locking system using the
localStoragemechanism to prevent two different instances oftelemetry(which could run in parallel from different windows/tabs in a browser/electron environment).This, combined with #28, should solve most concurrency issues on the
telemetrypackage.Remaining issue
There's still one remaining potential issue which is minor (at least compared to this one and the one fixed by #28):
Any event happening while the reporting is being done is lost: Since happens because we're clearing all the events once the current events are being reported.
In order to fix this, we have two options:
Anyways, we should fix this remaining issue as a separate PR since it's orthogonal to this work.