Prevent session from being destroyed when errors are raised during cleanAppData #7690
Conversation
|
@bridiver something is broken if an exception occurs, and I agree that just logging masks the problem... would we be able to hook into the error reporting for these instances? I committed a part 2 which we can consider also (with or without the first commit) which renames the session file in a worst case scenario (so that the user's session doesn't get overwritten with a default state when they close the browser) |
|
we talked about keeping a versioned history so you can rollback, but really we just need to write this to UserPrefs so it updates individual keys instead of writing the entire file every time which would be pretty easy to do |
|
@bridiver the UserPrefs approach would be nice... I'm not aware of how little or how much work is needed to take advantage of that. It seems like it would have a large impact though (lots of edge cases; upgrading from VERY old versions to new version, migrations from newer to older versions, older to newer, etc). The changes in this PR are pretty low-tech and can start helping immediately (although, I need to test them out more). I can definitely understand how the try/catch can mask the error (which is not good and wasn't my intention)... but I'd at least like to advocate for accepting the session file backup when an error is caught (while loading) |
|
Actually why is |
|
Good question @diracdeltas; it was added in 1201779 by @bbondy |
|
@lukemulks just lost his session file and I believe the fixes here could prevent it from happening again @bridiver instead of eating the exceptions, what would you think about me reworking this PR to only keep the rename/preserve the session file part (when exceptions are caught)? Whatever the root cause is (corruption, etc), it causes the session to fail parsing. This causes the app to have a default state, which overwrites the existing session file (permanently erasing the user's session) |
|
@aekeus would we be able to plug into our error reporting services to anonymously report the errors which happen (instead of printing them out to the console?). An alternative I guess could be that we store persistent log files which we could ask users for when debugging |
|
Before reworking this, I want to get some tests in place. I'll be submitting another PR later with tests for our existing session code. If you'd like a preview, here's the first commit: |
bsclifton
changed the title
bsclifton
changed the title
app/sessionStore.js
Outdated
|
|
||
| console.log('An error occurred. For support purposes, file "' + src + '" has been renamed to "' + dest + '".') | ||
|
|
||
| fs.rename(src, dest, (err) => { |
There was a problem hiding this comment.
this introduces a potential race condition between the rename and the next save. It's unlikely to happen, but I don't think we want to add any more potential race conditions so I think this should be a sync operation and we should use copy instead of rename in case the bug is somehow related to rename
|
Per discussion w/ @bridiver, we may also want to implement flushing when writing session to disk |
| } catch (e) { | ||
| // Session state might be corrupted; let's backup this | ||
| // corrupted value for people to report into support. | ||
| module.exports.backupSession() |
There was a problem hiding this comment.
There's more to do here as per discussions but doesn't need to all be done in this bug.
But I think this will try to do a sync copy on disk on first startup which would slow down startup. Could you verify if that's the case?
|
to me the lack of fsync seems like the most likely culprit given how randomly these things occur |
|
The test plan would be to force a BSOD? |
Taking a step towards #5512 - Take most of the session loading logic out of the massive try/catch - This adds 13+ tests to ensure behavior works as expected Before % Stmts: 52.65 % Branch: 45.77 % Funcs: 54.05 % Lines: 53.21 After % Stmts: 55.33 % Branch: 48.59 % Funcs: 56.41 % Lines: 56.41 - If file fails JSON.parse, a backup is saved. This is written synchronously Auditors: @bbondy, @bridiver Test Plan: `npm run unittest -- --grep="sessionStore unit tests"`
Taking a step towards #5512
unit test coverage
Before
% Stmts: 52.65
% Branch: 45.77
% Funcs: 54.05
% Lines: 53.21
After
% Stmts: 55.33
% Branch: 48.59
% Funcs: 56.41
% Lines: 56.41
overview
Losing your session sucks- every report by a user means they lost all of their bookmarks and likely didn't have a backup. This PR takes a small step to help prevent session from being destroyed. Here's how it works
Auditors: @lukemulks, @diracdeltas, @bridiver, @bbondy