electron-userland / electron-json-storage

:package: Easily write and read user settings in Electron apps
1.43k stars 79 forks source link

On rare occasion file written by electron-json-storage is zeroed out. #123

Closed Nantris closed 4 years ago

Nantris commented 5 years ago

I've been struggling for months to find the root cause of this issue, but it occurs so rarely I've been unable to pin it down.

I think this happens mostly on Mac, though perhaps my sample size is just too small to accurately tell if environment is a significant factor. This still happens on 4.1.5.

jviotti commented 5 years ago

Hey @Slapbox ,

Can you describe a bit more about how your application uses this library, as that might help thinking about what could be going wrong? Are you using it from the main or renderer process (or both)? Is more than one process potentially writing to a key at the same time? Do you usually update different keys or the same one? What kind of data are to storing? What's usually the size of the data you store?

Nantris commented 5 years ago

Hey @jviotti thanks for your response.

Are you using it from the main or renderer process (or both)? Just the renderer process.

Is more than one process potentially writing to a key at the same time? Nope.

Do you usually update different keys or the same one? I'm overwriting the entire file currently using this code, wired up via a Redux store enhancer. I'm using a lazy handling

What kind of data are to storing? I'm not sure how to answer this in a helpful way.

What's usually the size of the data you store? The largest dataset that I've seen the issue with was 50kb, although I've stored up to 2mb without issues.

When the user quits the app I lazily wait before quitting for an amount of time equal to my throttle timeout in that gist linked above. I thought just increasing that lazy timeout to a couple hundred milliseconds more than the throttle would fix these rare issues, but it didn't seem to.

A lot of the difficulty here is that I've never been able to produce this outside of development, but a couple users have seen it in production so it's been pretty tough to debug relying on user reports. Any ideas are appreciated!

jviotti commented 5 years ago

@Slapbox Thanks for the detailed info!

I'm not sure how to answer this in a helpful way.

I'm more interested in the structure of the data (i.e. a big object or many arrays) than about the content.

By looking at your gist, there is nothing waiting for the .set() function to complete: https://gist.github.com/Slapbox/a7d055cf4193328c2fe9d1d7cf88c0b8#file-storage-js-L10. If for whatever reason (like something in the OS causing the writes to slow down significantly, like some anti viruses), the next .set() may kick in before the previous one finished.

This module should protect against that out of the box using the lockfile dependency, but we might have found an edge case in either electron-json-storage or lockfile where the locking doesn't happen as expected.

What happens if the you reduce the throttle value? Can you try various lower numbers, update your Redux store continuously, and see if you can reproduce?

Also, what does "zero out" mean exactly? Is the file size getting to 0 bytes? Is it an empty object? An empty string? A corrupted object?

How are you noticing that the file is zeroed out? If there another process that attempts to read back? You are continuously overwriting the same file on each change, so even if a write fails, the next one will recover it, right?

Nantris commented 5 years ago

@jviotti thanks for your details response! I'll have to do some tinkering to find out the answers. You're right nothing is waiting for the set operation and it hadn't occurred to me how much something like an AV might slow things down.

When the file is "zeroed out" it's zero bytes, but I can't recall with certainty if there's any characters inside. I'm guessing not. The reading back happens only when the app is restarted, so if it is producing failures at times other than at app exit I've not noticed because of this.

I'm going to be away from my work for a few days but when I get back I'll take a look at the issue with your suggestions in mind and update with what I find. Thanks again!

(Oh and regarding data, most of the data is stored as an array of relatively light objects (2-4kb mostly) under a single key, although the fact that I'm overwriting the entire file probably makes this a moot point.)

jviotti commented 5 years ago

Hey @Slapbox,

Did you have time to play around with this?

When the file is "zeroed out" it's zero bytes, but I can't recall with certainty if there's any characters inside. I'm guessing not.

A completely zeroed out file would be pretty weird; I'd expect an empty object, or some random corrupted stuff. Maybe it'd make sense to log that last modification date of the file on an error handler when this problem happens?

jaruba commented 5 years ago

Sounds like: https://github.com/nathanbuchar/electron-settings/issues/110

I'm actually running around through github trying to find a module that saves json to a file that DOESN'T destroy the entirety of the data on restarts or unexpected shutdowns.

Even with atomic saving implemented in electron-settings, I'm still encountering such issues sometimes.

jviotti commented 5 years ago

@jaruba Do you notice any patterns when this happens? Is it mostly a particular OS (with a particular version)? A similar setup?

Can you actively reproduce by restarting or shutting down the computer in the middle of a write?

As a side note, v4.1.6 of this module implements atomic writes.

jaruba commented 5 years ago

I've added atomic write to a fork of electron-settings and used the version that has it for months now, it used to happen more when atomic write wasn't implemented, but my pc used to have unexpected shutdowns more often too (a hardware issue that i fixed in the meanwhile).

My application runs on start-up, is generally kept running constantly and still seems to loose all data on restart (not always though), it still happens now that unexpected restarts rarely if ever happen (only in cases of power surges) and atomic save is used.

I normally use windows 10, but have 2 osx systems on hand and a ubuntu laptop. It doesn't seem like the OS is at cause here, i literally can't explain the issue, that's why I'm looking for alternatives.

I'm building a torrent client, persistent user data is a must in my app's case, and sql related databases feel like overkill at this stage, but if I can't find a suitable simplistic alternative that doesn't loose all data on restarts, I'm prepared to use a sql solution in the end.

jviotti commented 5 years ago

@jaruba That's very strange. Keep in mind that with something like sqlite, the data will still be in a single fail, so if there is something at the OS level, then the issue may persist.

These electron settings modules are all pretty much thin wrappers over the official Node.js file-system modules, so I'd research a bit if any other Node.js user is reporting similar issues on the Node.js file-system built-in module, as the issue might come from there.

Did you notice different behaviour on different Electron versions?

jaruba commented 5 years ago

I was using Electron v1.6.7, and although it makes sense that the fs module might be at fault here, i'm also using this in an extreme scenario in which my app is always running on the system. I've updated to Electron v3.1.3, and will continue tests on it. I'd avoid using Electron v4.x.x unless this issue is still present as usually the latest versions of Electron do more harm then good.

I'll keep you posted on my findings, although i'd need to test this for a long time to make sure of the results.

jviotti commented 5 years ago

@jaruba Sounds good. Here is an idea of something you can do to both try to mitigate the problem and get more info about the problem:

Maybe you can store your data twice (or more times) in different files, and you read from all of them every time you need to access the data. If one is empty, then you report the contents somewhere for debugging purposes, and you can still use the other file which will likely not be corrupted.

What do you think?

Nantris commented 5 years ago

Just wanted to update this issue to say that I'm pretty sure I've managed to avoid this issue by immediately flushing my throttled version of electron-json-storage when the app is ready to quit, and extending the app's time to quit by several seconds. It's not the cleanest solution, but no data loss for the past month. Still, the issue is very rare to begin with and perhaps I've avoided it by sheer chance.

jaruba commented 5 years ago

@Slapbox I not only understand ur predicament for a simple testing environment against this, but it also makes more sense to me then presuming there's a bug in the fs module.

Unexpectedly interrupting a write with an app quit could definitely cause this issue, and definitely did so in the past, although atomic saving should safeguard against such a scenario.

I'm not throttling writes though, although that would make sense for my case too as during a torrent download writes can be frequent.

I hope u keep us posted about ur results, as i'm interested how it turns out for ur case.

Nantris commented 5 years ago

Just an update, I hit this again in development. The file is totally empty, no {} or anything.

255kb commented 4 years ago

I encountered the same problem on Mockoon (https://github.com/mockoon/mockoon). File gets emptied, apparently after a save or after closing the application. I only noticed this after the next launch, so I cannot know for sure what the cause is and I don't have any meaningful logs. It happened to me once on Win 7 with the packaged app (after a savage computer shutdown, but the application wasn't saving) and on Windows 10 during development (unpackaged app). The data saved was always different, most of the time simple. While the underlying cause might be external to this library, it would be great to have this PR merged: https://github.com/electron-userland/electron-json-storage/pull/128

Nantris commented 4 years ago

I didn't know #128 existed. I would really like to see that merged.

jviotti commented 4 years ago

I'm shipping a validate: true opt-in option in .set() as part of v4.2.0 that will make .set() read back the keys after a little while and retry (for a limited number of times) if the contents do not match. Use this with care though, as you might get into some concurrency problems if more than one process writes keys at the same time and try to validate what each other wrote.