File Recovery Service #11828
File Recovery Service #11828
Conversation
…so that other event handlers have the chance to execute even if the user doesn't choose an option in the message box. This will allow us to recover files when a window crashes.
| @@ -360,7 +360,6 @@ class Project extends Model | |||
|
|
|||
| addBuffer: (buffer, options={}) -> | |||
| @addBufferAtIndex(buffer, @buffers.length, options) | |||
| @subscribeToBuffer(buffer) | |||
There was a problem hiding this comment.
Apparently, we were subscribing to buffers twice, once here and once in addBufferAtIndex.
|
/cc: @atom/core for 👀 |
|
This is lookin' great, @as-cii, thanks very much for taking this on. I have a couple questions / comments:
|
| this.recoveryPathsByWindowAndFilePath.get(window).set(path, recoveryPath) | ||
|
|
||
| if (!this.crashListeners.has(window)) { | ||
| window.on('crashed', () => this.recoverFilesForWindow(window)) |
There was a problem hiding this comment.
Do you think we should remove the listener when the window closes? Would the callback's reference to window retain it and keep it from being removed from the WeakSet?
There was a problem hiding this comment.
That's a great point, thanks! 💯
Now that I think of it, @as-cii didn't you experiment with saving via a different process and it was too slow? The suggested process where the renderer process creates a temp file somewhere containing the data to be saved and the main process takes that and copies it into the real save location isn't that different from what we have here... except maybe we don't have to worry as much about crashes? What do you think? |
What makes me nervous about this solution is handing possibly huge chunks of memory to another process through ipc. Maybe this isn't a huge deal in practice, but we ran into some troubles when sending lots of contents down an ipc channel in the past (for
Yeah, in that case we also had the huge overhead of spawning a new process, whereas here we have the main process already running, which makes it an (almost) zero cost facility that we can control through ipc.
Good call, @BinaryMuse! 👍 |
| } | ||
|
|
||
| recoverSync () { | ||
| fs.writeFileSync(this.originalPath, fs.readFileSync(this.recoveryPath)) |
There was a problem hiding this comment.
If the files are large, will storeSync and recoverSync greatly increase memory usage? I wonder if reading some number of bytes at a time, or using some synchronous version of pipe, would be better?
as-cii
force-pushed
the
as-file-recovery-service
branch
from
72f1ae9 to
6c34844
Compare
as-cii
force-pushed
the
as-file-recovery-service
branch
from
bd47130 to
8355e7f
Compare
|
🚀 Thanks everyone for the great feedback! ❤️💚💙💜 |
After shipping #11828 Atom always creates a backup file in the `~/.atom/recovery` directory, trying to restore it automatically via the main process in case there is a hard crash on the renderer process. This approach should be resilient enough to allow us to delete this config option altogether. We can still keep it for a while in text-buffer, in case we need to use it again at some point, but it's probably fine to remove it from there too at some point in the future.
Depends on #11826 (to run spec/browser/file-recovery-service.spec.js).Fixes #11708.
Recently, we have been observing several issues related to losing data when Atom hard-crashes. We think that this problem manifests whenever the following happens:
fs.writeFileSync(path)gets called.fd = fs.openSync(path, "w")gets called. This causes the file to be truncated.fs.writeSync(fd, contents)has the chance to run, a hard crash occurs.The scenario described above leaves the file in a "truncated state", thus erasing all of its contents. In the past we have experimented with backup files to solve this issue, but that creates other problems such as paths that are too long on Windows, and unnecessary events triggered on fs watchers that are monitoring the saved file's directory. It turns out that atomic saves are not a good trade-off either, as described on #3158 (comment).
With this pull-request we want to provide a file recovery service running on the main process that keeps track of pending changes, and restores files to their original state when a renderer process crashes while a save is in flight. If the recovery is unsuccessful for any reason (e.g. problems with permissions, unmounted network drives, etc.), we log a message like the following in the system console and in a message box too, so that users can perform the recovery themselves:
By default we will use
~/.atom/recoveryas our recovery directory, where we temporarily store a backup of the saved file and delete it upon successful save/recovery. The nice thing about using the file system as our recovery storage (as opposed to IndexedDb), is that users can inspect it pretty easily and copy/paste content via Terminal.app or Finder.app (or their equivalent on other platforms).As a convention, backup files stored under the recovery directory, will have the following format:
Where:
originalFilenameis the name of the file without its extension. This string will be truncated if it's longer than34characters.randomHexNumberis a randomly generated hexadecimal number. This string will be 6 characters long by default.originalExtensionis the extension of the file.This should be long enough to make filenames readable (and more inspectable), but short enough to avoid path length issues on platforms like Windows.