connect-pg-simple takes 10s to shutdown

[alxndrsn]

Node version: 10.16.0 Sails version (sails): 1.2.3 ORM hook version (sails-hook-orm): DB adapter & version (e.g. sails-mysql@5.55.5): sails-postgresql 1.0.2

---

connect-pg-simple takes 10s to shutdown at the end of tests.

Example project at: https://github.com/alxndrsn/sailsjs-connect-pg-simple-shutdown-delay

To recreate:

git clone https://github.com/alxndrsn/sailsjs-connect-pg-simple-shutdown-delay
cd sailsjs-connect-pg-simple-shutdown-delay

$ yarn test
...
  connect-pg-simple leaves a hanging promise
    ✓ leaves a hanging promise...

  1 passing (542ms)

Done.
Done in 11.73s.

$ IN_MEMORY_SESSIONS=true yarn test
...
  connect-pg-simple leaves a hanging promise
    ✓ leaves a hanging promise...

  1 passing (530ms)

Done.
Done in 1.70s.

[sailsbot]

@alxndrsn Thanks for posting! We'll take a look as soon as possible.

In the mean time, there are a few ways you can help speed things along:

• look for a workaround. (Even if it's just temporary, sharing your solution can save someone else a lot of time and effort.)
• tell us why this issue is important to you and your team. What are you trying to accomplish? (Submissions with a little bit of human context tend to be easier to understand and faster to resolve.)
• make sure you've provided clear instructions on how to reproduce the bug from a clean install.
• double-check that you've provided all of the requested version and dependency information. (Some of this info might seem irrelevant at first, like which database adapter you're using, but we ask that you include it anyway. Oftentimes an issue is caused by a confluence of unexpected factors, and it can save everybody a ton of time to know all the details up front.)
• read the code of conduct.
• if appropriate, ask your business to sponsor your issue. (Open source is our passion, and our core maintainers volunteer many of their nights and weekends working on Sails. But you only get so many nights and weekends in life, and stuff gets done a lot faster when you can work on it during normal daylight hours.)
• let us know if you are using a 3rd party plugin; whether that's a database adapter, a non-standard view engine, or any other dependency maintained by someone other than our core team. (Besides the name of the 3rd party package, it helps to include the exact version you're using. If you're unsure, check out this list of all the core packages we maintain.)

  ---

Please remember: never post in a public forum if you believe you've found a genuine security vulnerability. Instead, disclose it responsibly.

For help with questions about Sails, click here.

[alxndrsn]

It seems you can work around this by manually destroying the session postgresql pool, using sails.config.session.store.close();.

In the example project, this can be done by changing test/config.js to:

const sails = require('sails');

before(done => {
  sails.lift({}, err => {
    if (err) { return done(err); }
    return done();
  });
});

after(done => {
  sails.lower(() => {
    sails.config.session.store.close();
    done();
  });
});

[johnabrams7]

@alxndrsn Thanks for exploring this shutdown delay with repro steps and a workaround. At the moment, our primary adapter in development sails-sql will improve upon sails-postgresql so this will be considered among development plans. Any further input of related applications / experiences from the community are welcome.

[alxndrsn]

@johnabrams7 I'd be interested to hear what sails-sql will do differently to sails-postgresql; is there any documentation? Will it handle sessions, or will connect-pg-simple still be required?

[johnabrams7]

@alxndrsn - So far, Postgres support in sails-sql won't be entirely different from the sails-postgresql adapter to start and will require Node v8+. connect-pg-simple will still be required as far I know. It's more of an effort to bring all the SQL adapters together to allow for more efficient development, releases, db flexibility. I know mysql and mssql have had more improvements in sails-sql, but postgresql improvements will come in good time.

[alxndrsn]

This also prevents sails console or sails lift from shutting down quickly.

Again this can be worked around by calling sails.config.session.store.close(); in the callback from sails.lift() in app.js:

// Start server
sails.lift(rc('sails'), err => {
  if(err) {
    console.error('Failed to lift app:', err);

    try {
      sails.config.session.store.close();
    } catch(err2) {
      console.error('Error caught trying to shut down postgres session store:', err2);
    }

    return;
  }

  sails.log.verbose('App lifted successfully.');
});

[johnabrams7]

@alxndrsn Appreciate the update & workaround! Will get this documented in the postgresql development notes.

[alxndrsn]

@johnabrams7 would it be suitable to add the following to sails-postgresql?

process.on('SIGTERM', () => {
  try {
    sails.config.session.store.close();
  } catch(err) {
    console.error('Error caught trying to shut down postgres session store:', err);
  }
});

If so, any pointers where would be a suitable place to put this?

[johnabrams7]

@alxndrsn Actually, sorry just realized connect-pg-simple is used as the session hook and sails-postgresql honestly has nothing to do with it 😄. This being said, a solution would reside in a change to connect-pg-simple -OR- the session hook itself in Sails without changing connect-pg-simple. If you have any ideas for that let us know! 👂

[alxndrsn]

@johnabrams7 is there a way to add a beforeShutdown function from the session hook?

[johnabrams7]

@alxndrsn Yes, you could override the session hook by creating api/hooks/session/ and copying what’s in sails core + modifying the teardown function.