fix: aliways refresh session

lib/session.js

@@ -335,8 +335,23 @@ module.exports = function (options) {
       result.isNew = true;
     }
 
-    yield next;
-    yield refreshSession.call(this, this.session, result.originalHash, result.isNew);
+    // make sure `refreshSession` always called
+    var firstError = null;
+    try {
+      yield next;
+    } catch (err) {
+      debug('next logic error: %s', err.message);
+      firstError = err;
+    }
+    // can't use finally because `refreshSession` is async
+    try {
+      yield refreshSession.call(this, this.session, result.originalHash, result.isNew);
+    } catch (err) {
+      debug('refresh session error: %s', err.message);
+      if (firstError) this.app.emit('error', err, this);
+      firstError = firstError || err;
+    }
+    if (firstError) throw firstError;
   }
 
   /**

package.json

@@ -15,25 +15,26 @@
   "author": "dead_horse <dead_horse@qq.com>",
   "dependencies": {
     "copy-to": "~2.0.1",
-    "crc": "~3.3.0",
+    "crc": "~3.4.0",
     "debug": "*",
-    "parseurl": "~1.3.0",
-    "uid-safe": "~2.1.0"
+    "parseurl": "~1.3.1",
+    "uid-safe": "~2.1.1"
   },
   "devDependencies": {
     "autod": "~2.1.3",
     "blanket": "*",
     "contributors": "*",
     "istanbul-harmony": "*",
-    "koa": "~1.1.2",
-    "koa-redis": "~1.0.1",
-    "mm": "~1.3.5",
+    "koa": "~1.2.0",
+    "koa-redis": "~2.1.1",
+    "mm": "~1.5.0",
     "mocha": "2",
-    "should": "~7.1.1",
+    "pedding": "^1.0.0",
+    "should": "~10.0.0",
     "supertest": "~0.13.0"
   },
   "engines": {
     "node": ">= 0.11.9"
   },
   "license": "MIT"
-}
+}

test/session.test.js

@@ -203,7 +203,7 @@ describe('test/koa-session.test.js', function () {
     });
 
     it('should regenerate existing sessions', function (done) {
-      var agent = request.agent(app)
+      var agent = request.agent(app);
       agent
         .get('/session/get')
         .expect(/.+/, function(err, res) {
@@ -223,5 +223,21 @@ describe('test/koa-session.test.js', function () {
         .get('/session/regenerateWithData')
         .expect({ /* foo: undefined, */ hasSession: true }, done);
     });
+
+    it('should always refreshSession', function(done) {
+      var cookie;
+      request(app)
+      .get('/session/get_error')
+      .expect(500)
+      .end(function(err, res) {
+        should.not.exist(err);
+        cookie = res.headers['set-cookie'].join(';');
+        should.exist(cookie);
+        request(app)
+        .get('/session/get')
+        .set('cookie', cookie)
+        .expect(/2/, done);
+      });
+    });
   });
 });

test/store.test.js

@@ -38,7 +38,7 @@ describe('test/store.test.js', function () {
       request(commonApp)
       .get('/session/get')
       .expect(500)
-      .expect('Internal Server Error', done);
+      .expect('session store is unavailable', done);
     });
 
     it('should get session ok when reconnect', function (done) {
@@ -58,7 +58,7 @@ describe('test/store.test.js', function () {
       request(commonApp)
       .get('/session/get')
       .expect(500)
-      .expect('Internal Server Error', done);
+      .expect('session store is unavailable', done);
     });
 
     it('should error when status is unavailable', function (done) {
@@ -67,7 +67,7 @@ describe('test/store.test.js', function () {
         request(commonApp)
         .get('/session/get')
         .expect(500)
-        .expect('Internal Server Error', done);
+        .expect('session store is unavailable', done);
       }, 200);
     });
 
@@ -117,9 +117,19 @@ describe('test/store.test.js', function () {
         .get('/session/get')
         .set('cookie', cookie)
         .expect(500)
-        .expect(/Internal Server Error/, done);
+        .expect('mock set error', done);
       });
     });
+
+    it('should handler session error when store.set error and logic error', function (done) {
+      mm(commonApp.store, 'set', function *() {
+        throw new Error('mock set error');
+      });
+      request(commonApp)
+      .get('/session/get_error')
+      .expect(500)
+      .expect('oops', done);
+    });
   });
 
   describe('defer session middleware', function () {
@@ -135,7 +145,7 @@ describe('test/store.test.js', function () {
       request(deferApp)
       .get('/session/get')
       .expect(500)
-      .expect('Internal Server Error', done);
+      .expect('session store is unavailable', done);
     });
 
     it('should get session ok when store.get error but session not exist', function (done) {
@@ -184,7 +194,7 @@ describe('test/store.test.js', function () {
         .get('/session/get')
         .set('cookie', cookie)
         .expect(500)
-        .expect(/Internal Server Error/, done);
+        .expect('mock set error', done);
       });
     });
   });

test/support/defer.js

@@ -24,6 +24,15 @@ app.outputErrors = true;
 app.keys = ['keys', 'keykeys'];
 app.proxy = true; // to support `X-Forwarded-*` header
 
+app.use(function*(next) {
+  try {
+    yield next;
+  } catch (err) {
+    this.status = err.status || 500;
+    this.body = err.message;
+  }
+});
+
 var store = new Store();
 app.use(session({
   key: 'koss:test_sid',

test/support/server.js

@@ -27,6 +27,15 @@ app.proxy = true; // to support `X-Forwarded-*` header
 
 var store = new Store();
 
+app.use(function*(next) {
+  try {
+    yield next;
+  } catch (err) {
+    this.status = err.status || 500;
+    this.body = err.message;
+  }
+});
+
 app.use(function*(next) {
   if (this.request.query.force_session_id) {
     this.sessionId = this.request.query.force_session_id;
@@ -85,6 +94,9 @@ app.use(function *controllers() {
   case '/session/get':
     get(this);
     break;
+  case '/session/get_error':
+    getError(this);
+    break;
   case '/session/nothing':
     nothing(this);
     break;
@@ -120,6 +132,12 @@ function get(ctx) {
   ctx.body = ctx.session.count;
 }
 
+function getError(ctx) {
+  ctx.session.count = ctx.session.count || 0;
+  ctx.session.count++;
+  throw new Error('oops');
+}
+
 function remove(ctx) {
   ctx.session = null;
   ctx.body = 0;