-1

I have a method that reads and write a log file, this method is called on every request by all users, then write the log the request path in a file. The questions are two:

  1. Is safe read and then write a file in async mode considering concurrency questions?
  2. If yes for the first question, the code bellow will work considering concurrency questions?
  3. If yes for the first question how I have to do?

Please, disregard exceptions and performance questions, this is a didactic code.


var logFile = '/tmp/logs/log.js';
app.get("/", function(req){
    var log = {path: req.path, date: new Date().getTime()};
    log(log);
});

function log(data){
    fs.exists(logFile, function(exists){
        if(exists){
            fs.readFile(logFile, function (err, data) {
                if (err){
                    throw err
                }
             var logData = JSON.parse(data.toString());
             logData.push(data);
             writeLog(logData);
            }); 
        }else{
            writeLog([data]);
        }

    });
     
}
function writeLog(base){
    fs.writeFile(logFile, JSON.stringify(base, null, '\t'), function(err) {
        if(err)
            throw err;
    }); 
}
Braiam
  • 1
  • 11
  • 47
  • 78
deFreitas
  • 4,196
  • 2
  • 33
  • 43
  • You're already reading and writing to it async - `readFileSync` and `writeFileSync` would do it synchronously. – brandonscript Oct 01 '15 at 19:10
  • Sorry if a I was not clear, I need to know if I can do it asynchronously – deFreitas Oct 01 '15 at 19:12
  • I'm saying you already are with the code you've written. However Benjamin makes an excellent point below. – brandonscript Oct 01 '15 at 19:13
  • FYI, your `throw err` will not do anything useful when throwing from an async callback. It will not get back to any exception handler you might have. The error will essentially just silently stop processing. Multiple nested async operations, really need something like promises to do proper error handling. – jfriend00 Oct 01 '15 at 20:29
  • @jfriend00, yes.. the point is that if can I do read/write in async mode in this case – deFreitas Oct 01 '15 at 20:42
  • I'm just explaining something else that is wrong with your code in case you also want to fix that. I was not commenting on the main point of your question. I thought you might want to know that your error handling does not work properly. – jfriend00 Oct 01 '15 at 20:44
  • in my case using Promise not work, I resolved the problem [here](https://stackoverflow.com/a/44609926/7830276) –  Jun 17 '17 at 22:29

2 Answers2

3

I strongly suggest that you don't just "log asynchronously" because you want the log to be ordered based on the order things happened in your app, and there is no guarantee this will happen that way if you don't synchronize it somehow.

You can, for instance, use a promise chain to synchronize it:

var _queue = Promise.resolve();
function log(message){
  _queue = _queue.then(function(){ // chain to the queue
    return new Promise(function(resolve){ 
      fs.appendFile("/tmp/logs/log.txt", new Date() + message + "\n", function(err, data){
        if(err) console.log(err); // don't die on log exceptions
        else resolve(); // signal to the queue it can progress
      });
    });
  });
}

You can now call log and it will queue messages and write them some time asynchronously for you. It will never take more than a single file descriptor or exhaust the server either.

Consider using a logging solution instead of rolling your own logger btw.

Benjamin Gruenbaum
  • 270,886
  • 87
  • 504
  • 504
  • Ok, So my file can receive disorderly logs but never will be corrupted? – deFreitas Oct 01 '15 at 19:25
  • @deFreitas with this solution it will always receive orderly logs - this is what the `.then` does, it queues the actions instead of just calling `writeFile`. – Benjamin Gruenbaum Oct 01 '15 at 19:25
  • Sorry, the question was for my implementation. – deFreitas Oct 01 '15 at 19:26
  • @deFreitas then yes, _probably_ although more writes will likely fail (won't corrupt though). Alternatively open a write stream for the file and append to that, that would also be good (if you close it on `process.on("uncaughtException"`). – Benjamin Gruenbaum Oct 01 '15 at 19:29
  • Your code never resolves the promise if there's a file error with `.appendFile()`, thus it just ends up queuing data forever. – jfriend00 Oct 01 '15 at 20:41
0

In you're example you're already using the Asynchronous versions of those functions. If you're concerned about the order of your operations then you should use the synchronous versions of those functions.

readFileSync
writeFileSync

Also to note, JSON.parse() is a synchronous operation.You can make this "asynchronous" using the async module and doing a async.asyncify(JSON.parse(data.toString()));.

As noted by @BenjaminGruenbaum, async.asyncify(); doesn't actually make the operation of JSON.parse(); truly asynchronous but it does provide a more "async" style for the control flow of the operations.

peteb
  • 18,552
  • 9
  • 50
  • 62
  • `JSON.parse` will stay sync if you use async.asyncify, that wouldn't help. It'll run synchronously, just later. – Benjamin Gruenbaum Oct 01 '15 at 19:15
  • @BenjaminGruenbaum definitely, but it would provide an async style control flow and would eliminate the need for a try/catch block as well. – peteb Oct 01 '15 at 19:24