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 upuse muon.file.writeImportant to write ledger files (#8602) #8605
Conversation
luixxiul
changed the title
|
Comments left; I also noticed that there are two places still using AtomicWriter(). Is that intended? (or should we switch everything to MuonWriter and remove the old code) |
| muon.file.writeImportant(path, JSON.stringify(payload, null, 2), (success) => { | ||
| if (!success) return console.log('write error: ' + path) | ||
|
|
||
| if ((quitP) && (!getSetting(settings.PAYMENTS_ENABLED)) && (getSetting(settings.SHUTDOWN_CLEAR_HISTORY))) { |
This comment has been minimized.
This comment has been minimized.
bsclifton
May 3, 2017
Member
this is kind of weird; so just to confirm... when Brave is exiting (user chose to quit) and payments aren't enabled / history is set to clear, we destroy the file by setting quitP to true and then logging something? I mention it's weird because the deletion only happens after the file is written to (versus just deleting the file).
This comment has been minimized.
This comment has been minimized.
mrose17
May 3, 2017
Author
Member
yes, it's weird: this deals with a race condition that @NejcZdovc found during testing.
it's done that way because the file may be in the process of being written when we want to delete it. in which case, if we unlink the file, we might just then go ahead and write it again. hence, always delete in this case...
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mrose17
May 3, 2017
Author
Member
i'm not sure whether it makes sense. it is certainly counter-intuitive: if you have successfully written the file, and the user is quitting, and the ledger isn't enabled, and history is to be deleted, then get rid of the synopsis file... reminiscent of https://www.youtube.com/watch?v=GoxcyV4DJuY
| return fs.unlink(path, (err) => { if (err) console.log('unlink error: ' + err.toString()) }) | ||
| } | ||
|
|
||
| if (ledgerInfo._internal.debugP) console.log('\nrenaming ' + path) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
no more atomicWriter ... 34d7eb9 ... ok, i'm done, i hope! |
|
LGTM |
mrose17 commentedMay 1, 2017
•
edited by bsclifton
Test Plan
no new tests. existing ledger tests should apply.
Description
git rebase -ito squash commits (if needed).Fixes #8602