Merge pull request #63 from runk/temp-files

Temp file to handle concurrent/cancelled downloads more gracefully

Author: runk

.travis.yml

@@ -1,5 +1,5 @@
 language: node_js
 node_js:
-  - "4.4"
+  - "4.8"
   - "6.2"
   - "7.6"

lib/cache.js

@@ -1,10 +1,10 @@
 'use strict';
 var path = require('path'),
   fs = require('fs'),
+  os = require('os'),
   crypto = require('crypto'),
   mkdirp = require('mkdirp');
 
-
 function Cache(opts) {
   this.opts = opts || {};
   this.opts.ttl = (opts.ttl || 1800) * 1000;
@@ -52,8 +52,9 @@ function Cache(opts) {
 
 
   this.read = function(key) {
-    var path = this.getPath(key),
-      file = fs.createReadStream(path.full);
+    var pathInfo = this.getPath(key),
+      file = fs.createReadStream(pathInfo.full);
+
     file.on('finish', function() {
       file.close(nop);
     });
@@ -62,23 +63,32 @@ function Cache(opts) {
   };
 
 
-  this.write = function(key) {
+  this.write = function(key, readStream, cb) {
     var locks = this.locks,
-      path = this.getPath(key);
+      pathInfo = this.getPath(key),
+      self = this;
 
-    // create lock
+    // Create a lock
     locks[key] = true;
 
-    mkdirp.sync(path.dir, 511); // 511 is decimal equvivalent of 0777
+    mkdirp.sync(pathInfo.dir, 511); // 511 is decimal equvivalent of 0777
 
-    var file = fs.createWriteStream(path.full);
-    file.on('finish', function() {
-      // release lock
+    // On top of locking mechanism, doing write to a temp location, and
+    // when it's finish moving the data file to its final destination.
+    var tmpPath = path.join(os.tmpdir(), pathInfo.file + '-' + Math.round(Math.random() * 1e9).toString(36));
+
+    var writeStream = fs.createWriteStream(tmpPath);
+    readStream.pipe(writeStream);
+
+    writeStream.on('finish', function() {
+      writeStream.close(nop);
+
+      // Release the lock, move data file to final destination.
       delete (locks[key]);
-      file.close(nop);
-    });
+      fs.renameSync(tmpPath, pathInfo.full);
 
-    return file;
+      self.meta(key, cb);
+    });
   };
 
 
@@ -90,7 +100,7 @@ function Cache(opts) {
       file = path.basename(key);
       // Cut the version suffix and file extension; only module name
       // should make the directory, make sure that there is no dot as
-      // directory name coming from the first characters of the fike name
+      // directory name coming from the first characters of the file name
       base = file.replace(/(-\d\.\d.\d)?\.tgz/, '').replace(/\./g, '-');
     } else {
       file = crypto.createHash('md5').update(key).digest('hex')

lib/proxy.js

@@ -137,14 +137,13 @@ exports.httpHandler = function(req, res) {
     var onResponse = function(err, response) {
       // don't save responses with codes other than 200
       if (!err && response.statusCode === 200) {
-        var file = cache.write(dest);
-        r.pipe(file).on('finish', function() {
-          cache.meta(dest, function(err, meta) {
-            if (err)
-              throw err;
-            respondWithCache(dest, cache, meta, res);
-          });
+        cache.write(dest, r, function(err, meta) {
+          if (err)
+            throw err;
+
+          respondWithCache(dest, cache, meta, res);
         });
+
       } else {
         // serve expired cache if user wants so
         if (exports.opts.expired && meta.status === Cache.EXPIRED)

test/cache.js

@@ -1,15 +1,19 @@
 'use strict';
 var assert = require('assert'),
   path = require('path'),
+  fs = require('fs'),
   rimraf = require('rimraf'),
   Cache = require('../lib/cache');
 
 describe('cache', function() {
 
   var opts;
   var filepath = path.join(__dirname, '/cache');
+
+  var dummy = 'Lorem ipsum dolor sit amet ...\n';
+
   beforeEach(function() {
-    opts = {path: filepath, ttl: 10};
+    opts = { path: filepath, ttl: 10 };
   });
 
   before(function(done) {
@@ -29,24 +33,47 @@ describe('cache', function() {
   });
 
 
-  describe('set()', function() {
-    it('should create new write stream', function() {
+  describe('write()', function() {
+    var readStream = fs.createReadStream(path.join(__dirname, 'dummy.data'));
+
+    it('should write the file', function(done) {
       var cache = new Cache(opts);
-      var file = cache.write('/-/foo/bar.dat');
-      file.end(new Buffer('This is a test'));
+      var key = '/-/foo/bar.dat';
+      var pathInfo = cache.getPath(key);
+      cache.write(key, readStream, function(err, meta) {
+        assert.equal(meta.size, 31);
+        assert.equal(meta.status, 4);
+        assert.equal(fs.readFileSync(pathInfo.full, 'utf8'), dummy);
+        done();
+      });
+    });
+
+    it('should handle locks', function(done) {
+      var cache = new Cache(opts);
+      var readStream = fs.createReadStream(path.join(__dirname, 'dummy.data'));
+      var key = '/-/foo/baz.dat';
+      var pathInfo = cache.getPath(key);
+      cache.write(key, readStream, function(err, meta) {
+        assert(!cache.locks[key], 'Lock should be released');
+        assert(fs.existsSync(pathInfo.full));
+        done();
+      });
+
+      assert(cache.locks[key], 'Lock should be set');
+      assert(!fs.existsSync(pathInfo.full));
     });
   });
 
 
-  describe('get()', function() {
+  describe('read()', function() {
     it('should create new read stream', function(done) {
       var cache = new Cache(opts);
       var readable = cache.read('/-/foo/bar.dat');
 
       readable.setEncoding('utf8');
       readable.on('data', function(data) {
         assert.equal(typeof data, 'string');
-        assert.equal(data.toString(), 'This is a test');
+        assert.equal(data.toString(), dummy);
         done();
       });
 
@@ -60,7 +87,7 @@ describe('cache', function() {
       var cache = new Cache(opts);
       cache.meta('/-/foo/bar.dat', function(err, meta) {
         if (err) return done(err);
-        assert.equal(meta.size, 14);
+        assert.equal(meta.size, 31);
         assert.equal(meta.type, 'application/octet-stream');
         assert.equal(meta.status, Cache.FRESH);
         done();

test/dummy.data

@@ -0,0 +1 @@
+Lorem ipsum dolor sit amet ...