-
Notifications
You must be signed in to change notification settings - Fork 12
Fix race condition when sending daily stats #25
Conversation
Wow this was fast! Thanks for working on this!! 🎉
I'm pretty sure that each electron window runs in a separate renderer process, in a similar way that chrome does for each tab, so I don't think that this solution would be effective. Instead, we can use the |
After looking at this I just got curious about This means that race conditions don't really exist at the moment: each window currently keeps its list of events but they "conflict" when checking the But if I'm not mistaken, this also means that we're currently losing all the events that haven't been sent once the Atom window gets closed, and since we're only sending events once Atom has been opened for 4h, we're losing any event from sessions shorter than 4h. Using a persistance adapter on LokiJS (as mentioned in the docs) would fix this specific problem, but then we would introduce the concurrency issues when having multiple windows open, since
So I see two potential paths forward (there may be more options):
|
🤦♂️I did not even realize that lokijs is in memory and doesn't persist. Thanks for bringing that up! A lesson to me to investigate tools more deeply.
This seems simpler to implement, since I can imagine more concurrency bugs that might shake out of the other approach, if we have multiple windows that are writing to disk when they close, and also trying to send stats. Since I don't love the plain indexdb api, I evaluated two other indexdb wrappers. pouchDBpros
cons
dexiehttps://dexie.org/docs/API-Reference#quick-reference pros
cons
Dexie is the clear winner here, so I'll start implementing a move from lokijs to dexie. @rafeca please let me know if I'm missing anything, or if you have any concerns about this approach. Thanks for your help! |
Hey @annthurium, sorry for the delayed response, I was on PTO until today 🏖 Thanks for investigating potential solutions and writing down the differences! I agree with your outcome and |
Closing this PR in favor of #29 |
Problem
@rafeca (and @shana) pointed out that there is a race condition with reporting stats. If we have multiple client windows open, we could end up sending the same stats twice. The db is shared across all client instances, and the database is cleared asynchronously after waiting for the request to finish.
Solution
The fix is to add an
isReporting
class attribute, which we set totrue
at the beginning of thereportStats
method and re-set tofalse
at the end. Since theStatsStore
class is a singleton, and nodejs event loop is single threaded*, hopefully this will prevent multiple running windows from trying to send data at the same time.Alternate approaches
I also considered making each window have its own db instance. Shana also pointed out that there might be race conditions on writes in #21. However, I did a little research and it looks like lokijs throttles automatic saves when these would cause overlaps with a save currently in progress. This prevents data loss at the cost of less frequent saves. So I think we're ok here, but I'm open to changing this if you disagree.