Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

File Recovery Service #11828

Merged
merged 34 commits into from
May 27, 2016
Merged

File Recovery Service #11828

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
ef3ab03
Emit {will,did}SavePath on `ipcMain` before/after a buffer is saved
May 23, 2016
049321a
:bug: :fire: Remove double subscription to the same buffer
May 23, 2016
de599f9
Make dialog asynchronous when a renderer process crashes
May 23, 2016
b58ce49
Create `FileRecoveryService` to restore corrupted files after a crash
May 23, 2016
770199c
:art: event.sender -> window
May 23, 2016
ca32c13
:fire: Unnecessary variable assignments
May 23, 2016
57195d7
:white_check_mark: Write specs for FileRecoveryService
May 23, 2016
97dd25d
:green_heart:
May 24, 2016
25fece4
:art: file-recovery-service-spec.js -> file-recovery-service.spec.js
May 24, 2016
858740c
:memo: Better description on spec
May 24, 2016
c7f4b33
Emit informative warning when a file can't be recovered
May 24, 2016
4f1efe6
:shirt: Fix linter errors
May 24, 2016
7a12984
:fire: Unused requires on specs
May 24, 2016
152c28a
Merge branch 'master' into as-file-recovery-service
May 24, 2016
e57b35f
Merge branch 'master' into as-file-recovery-service
May 24, 2016
b84feeb
Emit {will,did}SavePath events synchronously
May 24, 2016
3b4c101
Forget window when it gets closed
May 25, 2016
c8fae11
Handle recovery when many windows save the same file simultaneously
May 25, 2016
3ce7d0a
Merge branch 'master' into as-file-recovery-service
May 25, 2016
3030723
:shirt: Fix linting issues
May 25, 2016
a2a734a
Generate readable recovery filenames
May 25, 2016
c6a87b9
Add sinon
May 25, 2016
1a7858c
Log a more informative message when cannot recover a file
May 25, 2016
c2b01d5
Make coupling looser between the recovery service and the windows
May 25, 2016
8ba275a
:art: Move RecoveryFile down
May 25, 2016
49a603a
Show also a message box when recovery is not successful
May 25, 2016
3f8f3c9
:fire: Remove extra comma
May 25, 2016
8733b52
:fire: Extra imports
May 25, 2016
d8564ad
Be a little more defensive when retaining/releasing recovery files
May 25, 2016
aefcbcd
:fire: Remove unneeded WeakSet
May 25, 2016
6c34844
:bug: Don't try to recover the same file twice
May 26, 2016
df263a2
Return early when a recovery file can't be stored
May 26, 2016
5e0e65b
Merge branch 'master' into as-file-recovery-service
May 26, 2016
8355e7f
Use fs.copyFileSync for buffered copy
May 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
👕 Fix linter errors
  • Loading branch information
Antonio Scandurra committed May 24, 2016
commit 4f1efe6ef69854942c03cddc00a3d1421dc674ca
4 changes: 2 additions & 2 deletions spec/browser/file-recovery-service.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ describe("FileRecoveryService", () => {
let mockWindow, recoveryService, recoveryDirectory

beforeEach(() => {
mockWindow = new Emitter
mockWindow = new Emitter()
recoveryDirectory = temp.mkdirSync()
recoveryService = new FileRecoveryService(recoveryDirectory)
})
Expand All @@ -32,7 +32,7 @@ describe("FileRecoveryService", () => {
})

it("creates many recovery files and deletes them when many windows attempt to save the same file", () => {
const anotherMockWindow = new Emitter
const anotherMockWindow = new Emitter()
let filePath = temp.path()

fs.writeFileSync(filePath, "some content")
Expand Down
6 changes: 3 additions & 3 deletions src/browser/file-recovery-service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import fs from 'fs-plus'
export default class FileRecoveryService {
constructor (recoveryDirectory) {
this.recoveryDirectory = recoveryDirectory
this.recoveryPathsByWindowAndFilePath = new WeakMap
this.crashListeners = new WeakSet
this.recoveryPathsByWindowAndFilePath = new WeakMap()
this.crashListeners = new WeakSet()
}

start () {
Expand All @@ -29,7 +29,7 @@ export default class FileRecoveryService {
fs.writeFileSync(recoveryPath, fs.readFileSync(path))
Copy link
Contributor

@BinaryMuse BinaryMuse May 24, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we ensure this is written by flushing? Granted this would only be a concern if the main process crashes...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about flushing, as I don't think it's enough to ensure the file was persisted. From man 2 fsync:

     Specifically, if the drive loses power or the OS crashes, the application may find that only some or none of their data was writ-
     ten.  The disk drive may also re-order the data so that later writes may be present, while earlier writes are not.

     This is not a theoretical edge case.  This scenario is easily reproduced with real world workloads and drive power failures.

     For applications that require tighter guarantees about the integrity of their data, Mac OS X provides the F_FULLFSYNC fcntl.  The
     F_FULLFSYNC fcntl asks the drive to flush all buffered data to permanent storage.  Applications, such as databases, that require a
     strict ordering of writes should use F_FULLFSYNC to ensure that their data is written in the order they expect.  Please see
     fcntl(2) for more detail.

So we might use fcntl but that wouldn't be cross-platform and I am not sure that investing time in flushing data to disk is actually worth it. What I like about this solution is that, worst-case scenario, users can find the contents of their file under ~/.atom/recovery after rebooting their machine when e.g. the system crashes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that sounds reasonable to me. Would it be worth using the original file basename as part of the filename so that they can be more easily identified?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth using the original file basename as part of the filename so that they can be more easily identified?

My main concern there is path length on Windows, which is why I went with a 10-letter random filename for these recovery files. Introducing a variable-length file name could cause weird edge cases on Windows, although it's admittedly more user-friendly. Maybe we can truncate the filename at some arbitrary length, and add a random, fixed-size suffix string? I'll try that shortly.


if (!this.recoveryPathsByWindowAndFilePath.has(window)) {
this.recoveryPathsByWindowAndFilePath.set(window, new Map)
this.recoveryPathsByWindowAndFilePath.set(window, new Map())
}
this.recoveryPathsByWindowAndFilePath.get(window).set(path, recoveryPath)

Expand Down