When database is not available, fatal error should not propagate.

[asergeyev]

I'm new to node and may not have good way of describing this issue, bear with me please.

When get() called and database is suddenly unavailable it's probably not a best behavior to propagate error all the way and crush application.

I wondered if modifying get() behavior when query returns error is a good thing or no. I would agree that passing just a simple "not found" is likely not a best behavior and may confuse people but crashing everyone, even those users who not yet logged in seem worse to me.

[voxpelli]

This is likely do to an unhandled error event from the pg module as can be seen in the issue I referenced this one from.

This module should of course handle those error events when no external pg module has been provide, but currently – as stated in the referenced issue – there might be some possible challenges with that as well.

The easy fix would be to just add an empty error listener – like: pg.on('error', function () {});

You can do this yourself right now if you provide your own pg module through the pg option, but I will look into adding it as a default as well. You're welcome to send a PR if you want.

[asergeyev]

I would not go to PR, there is really too many things I'd like to understand first :) My first ever app with express, baby steps. What I have tried and found sufficient was this (repeating what I seen in your other function):

diff --git a/index.js b/index.js
index 27d2287..ec1dfaf 100644
--- a/index.js
+++ b/index.js
@@ -157,7 +157,10 @@ module.exports = function (session) {

   PGStore.prototype.get = function (sid, fn) {
     this.query('SELECT sess FROM ' + this.quotedTable() + ' WHERE sid = $1 AND expire >= NOW()', [sid], function (err, data) {
-      if (err) { return fn(err); }
+      if (err) {
+        console.log("Failed to fetch session info:", err.message);
+        return fn.apply(this, err);
+      }
       if (!data) { return fn(); }
       try {
         return fn(null, ('string' === typeof data.sess) ? JSON.parse(data.sess) : data.sess);

But I got no idea why it's helping :)

[voxpelli]

That shouldn't really change anything apart from the fact that fn.apply() requires the second argument to be an array and you have sent in an Error object instead, which when I tested it would mean that no parameter at all is sent to the callback, which would then mean that Express session would think that everything went fine with the DB call and that just no existing session were found: https://github.com/expressjs/session/blob/master/index.js#L405

It shouldn't make a difference in whether an exception is thrown or not so the application should still crash as much or as little anyway (unless you have set up some Express error handling that throws those errors as exceptions rather than return 500-errors to the client, which sounds pretty unlikely).

[asergeyev]

Ack. Thanks for detailed info.