electron-userland / electron-json-storage

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

File contents empty on read sometimes #107

Closed mikeseese closed 6 years ago

mikeseese commented 6 years ago

This may be related to #40, but #40 seems to relate more to writing.

My issue is that get sometimes reads an empty file (e.g. https://github.com/electron-userland/electron-json-storage/blob/master/lib/storage.js#L133, object is an empty string). This causes JSON.parse() to fail because there is not JSON object in the string.

The contents of the file are still there, but for some reason readFile is returning an empty string. I verified that it's reading the correct file, and cat file.json returns the expected contents.

I'm running on Linux with v4.1.0

mikeseese commented 6 years ago

Changing https://github.com/electron-userland/electron-json-storage/blob/master/lib/storage.js#L130-L144 to

    function(fileName, callback) {
      const object = fs.readFileSync(fileName, {
        encoding: 'utf8'
      }).toString();

      callback(null, object);
    }

fixes the issue strangely

mikeseese commented 6 years ago

I then tried to recreate the issue in a test to see if fs was the issue:

const fs = require("fs");
const async = require("async");

const total = 1000;
let countFail = 0;

async.timesSeries(total, (n, callback) => {
  fs.readFile("/home/mike/.config/Electron/global/Settings.json", {
    encoding: "utf8"
  }, function(err, object) {
    if (err) throw err;

    if (object === "") {
      countFail++;
    }

    console.log(object);

    callback();
  });
}, () => {
  console.log(countFail);
});

but alas, no failures. I then thought perhaps it was Electron (which does package it's own nodejs, and apparently wraps the fs functions). So I put this test in my electron application to use electron's node, and that wasn't the issue.

The next test is to use electron-json-storage in a non electron application (if that's feasible, I don't see why not) Edit: Looks like this library does depend on using within an Electron app/context (which is totally cool), but this starts to leave me lost as to why this error is happening

mikeseese commented 6 years ago

I also tried manually changing the package.json (followed by a local npm install in node_modules/electron-json-storage) to incorporate the changes from https://github.com/electron-userland/electron-json-storage/commit/7f035f2cf54038c9c45026d977d8556d45706fc9 and alas no luck

A this point I'm going to wait for feedback :) I've done all I can think of for now

mikeseese commented 6 years ago

@jviotti would you be opposed to a PR that uses the readFileSync method instead of readFile in storage.js:get()? It seems this fixes the issue, despite the fact I cannot determine the root cause. Thanks!

jviotti commented 6 years ago

Hi guys. Good news! I believe I fixed this issue on this PR: https://github.com/electron-userland/electron-json-storage/pull/110. Would you mind giving a shot to confirm the issue is ruled out before merging and publishing?

Basically the problem was that the locking module I was using was not doing real locking, but just some checks in-memory within the same process, so writing the file from more than one process would trigger the issue.

Let me know!

jviotti commented 6 years ago

Oops! I got the PR link wrong. I edited the original message, but just in case, here is the right one: https://github.com/electron-userland/electron-json-storage/pull/110

mikeseese commented 6 years ago

Well bummer; for some reason I cannot reproduce the original problem on 4.1.0. however proper-locking branch does not break things. From my perspective, I guess I consider this closed after #110 is merged since the original issue could potentially be a locking problem.

I changed code since I first posted this issue so it's possible that caused the issue to not be apparent.

Thanks for looking into it!