fs.writeFile() seems to not be running the callback suddenly

[Kariah_Petrille]

So, I have a bunch of JSON files in my project and use fs.writeFile() a lot to keep them updated. In the last few days, my saveFile function hasn’t been commenting properly:

async function saveFile(filename, obj, comment){
  //console.log(comment);
  fs.writeFile(filename, JSON.stringify(obj, null, 2), function (err){
    if (err){ 
      throw err;
    }
    if(comment != null && comment != "" && comment!= undefined){
      console.log(comment);
    }
    return;
  });
}
 

It was working fine before. It generally seems to be saving properly, but the lack of comments is wigging me out.

[Kariah_Petrille]

I’m going to have that “it seems to be working” back. It is only working some of the time, and I’m not getting any kind of error to alert me if the save isn’t happening.

[wh0]

the saveFile snippet looks like it would work, and I feel like you could prove to yourself that it works fine with a small test script.

I’d suggest looking at either the code responsible for using saveFile or the container disk performance.

for one thing, saveFile is declared as async, but it doesn’t internally await anything.

[Kariah_Petrille]

I tested it with a single call and it worked, but it was hit or miss whether it was saving in the larger calls. I’m not sure why that would stop it from running the function though, especially when it’s been doing fine for months.

I was under the impression that writeFile was an async method itself. I’ll remove the async declaration and see if that helps.

[wh0]

how large of a file makes it not work?

[Kariah_Petrille]

It worked OK when generating a new file with 1 line of code, but it was hit or miss saving the task file which is 24KB ~ 700 lines of json code atm.

I think I may have tracked down the problem. You’d mentioned that nothing was awaiting in the saveFile function. I had made some changes trying to make sure that it wasn’t trying to messing up the files by trying to write to the same file at the same time. It did at some point have a declaration to await the fs.writeFile function, but it didn’t work like I was expecting so I rolled it back to the original code.

I tried switching to writeFileSync per your recommendation to make it a synchronous function and got an error about that function not being defined, so I checked the fs declaration and it was set to fs=require(“fs”).promise because that was what I was told to do and I forgot about it when I was resetting the main call during the rollback.

I took that out and wrapped the writeFile call in a promise to try to get what I was going for:

async function saveFile(filename, obj, comment){
  return new Promise ((resolve, reject)=>{
    fs.writeFile(filename, JSON.stringify(obj, null, 2), function (err){
      if(err){
        reject(err);
      }
      if(comment != null && comment != "" && comment!= undefined){
        console.log(comment);
      };
      resolve(true);
    });
  }).then((saved)=>{
    return saved;
  }).catch((err)=>{
    console.log(err);
    return false;
  });
}

I’m going to run with this for a bit and see if it fixes the issue and report back.

[wh0]

oh yeah, require('fs').promises.writeFile indeed doesn’t take a callback. that would explain why the callback was never called, as it was completely ignored.

I had made some changes trying to make sure that it wasn’t trying to messing up the files by trying to write to the same file at the same time.

definitely for that, ccing @khalby786 who maintains a json file saving library

fs = require('fs').promises goes with await fs.writeFile(...) and async function saveFile(...) { ... }. that’ll make saveFile wait for the write to finish.

[Kariah_Petrille]

So far so good! I ran a couple tests and it seems to be working good.

@khalby786 Do you see any problems arising from leaving the function as is?

[khalby786]

@Kariah_Petrille

So far so good! I ran a couple tests and it seems to be working good.

@khalby786 Do you see any problems arising from leaving the function as is?

Sun Tzu, a Chinese military general and strategist, famously known as the author of Art of War, once said

If your code works perfectly, it’s best not to touch it.

---

Here’s my take on the whole thing:

@Kariah_Petrille

In the last few days, my saveFile function hasn’t been commenting properly

You mentioned that your function wasn’t commenting properly, but what if the file was written and something held up the callback, preventing the comment? More on the blocking below.

@wh0

fs = require('fs').promises goes with await fs.writeFile(...) and async function saveFile(...) { ... }. that’ll make saveFile wait for the write to finish.

@Kariah_Petrille

async function saveFile(filename, obj, comment){
  return new Promise ((resolve, reject)=>{

As @wh0 mentions above, nesting your writeFile in a promise, which is also nested in an async function, is heavily unnecessary. Keeping the original async saveFile function, I’d recommend

const fs = require('fs').promises;

async function saveFile(filename, obj, comment) {
  try {
    await fs.writeFile(filename, JSON.stringify(obj, null, 2))

    if (comment != null && comment != "" && comment != undefined) {
    // or if (comment), which is the same thing
      console.log(comment);
    }
  } catch (err) {
    // console.error(err)
    throw err
  } 

  return;
}

Alternatively, suppose you’d like to use the original writeFile code. In that case, you should run it in a normal synchronous function, as in function saveFile without the async, which @wh0 once again rightfully points out.

@Kariah_Petrille

I tried switching to writeFileSync per your recommendation to make it a synchronous function

Let’s talk about writeFile and writeFileSync.

writeFile is asynchronous, meaning it will run when it gets a chance while your program continues to do other stuff - we call this “non-blocking”.

On the other hand, writeFileSync is synchronous and blocking, which means that your program is going to “stop the car, stop everything, focus on the file write and nothing else”. This pauses the execution of the program to ensure your file gets written, before moving onto the rest of the program. It’s also faster, but at the cost of pausing execution. You won’t really notice the difference between writeFileSync and writeFile, especially in terms of speed and blocking, unless you’re dealing with large files.

I would strongly recommend avoiding writeFileSync, especially if you’re dealing with multiple file writes, especially on a (Node) server. I remember a StackOverflow post that said writeFile would be faster if you did not wait for the callback, but I’m unable to verify this claim.

@Kariah_Petrille

the task file which is 24KB ~ 700 lines of json code atm.

Whether 24kB is a large file or not is up for debate. For reference, 500 placeholder comments is roughly around 41kB, so the task file would be half of that.

tl;dr: if this runs perfectly fine, go ahead with it.

[Kariah_Petrille]

Final Update:
Cleaned my code up as recommended (it just seemed like it was neater and did the same thing) and added an error message.

/*
  saveFile
    saves JSON state of a given object to the file
    Returns true if the save was successful and an error if not
  filename -- string of file location
  obj -- JSON object to be saved
  comment -- string indicating what to log on the console. May be null
*/
async function saveFile(filename, obj, comment){
  
  return await fs.writeFile(filename, JSON.stringify(obj, null, 2)).then((saved)=>{
    if(comment){
      console.log(comment);
    }
    return true;
  }).catch((err)=>{
    return new Error("Could not save " + filename + ": " + err);
  });
}
//end saveFile

Thanks for your help @khalby786 and @wh0 !