search

balderdashy / sails

Realtime MVC Framework for Node.js
https://sailsjs.com
MIT License
22.84k stars 1.95k forks source link

sails.lower fails to exit correctly with redis session adapter #4170

Closed mjasnikovs closed 6 years ago

mjasnikovs commented 7 years ago

Sails version: v1.0.0-36 Node version: v8.3.0 NPM version: 5.4.0 Operating system: Windows 7

If session configuration contains Redis adapter sails.lower does not exit correctly. I think Sails keeps a connection to Redis alive and not close it.

const test = require('tape')
const sails = require('sails')

test('sails.lift', t => {
    sails.lift({
        hooks: {grunt: false},
        log: {level: 'warn'}
    },
    err => {
        t.equal(typeof err, 'undefined', 'server.lift err')
        t.end()
    })
})

test('sails.lower', t => {
    sails.lower(err => {
        t.equal(typeof err, 'undefined', 'server.lower err')
        t.end()
    })
})

Curent solution is to add

process.exit(0)
sailsbot commented 7 years ago

Hi @mjasnikovs! It looks like you missed a step or two when you created your issue. Please edit your comment (use the pencil icon at the top-right corner of the comment box) and fix the following:

As soon as those items are rectified, post a new comment (e.g. “Ok, fixed!”) below and we'll take a look. Thanks!

*If you feel this message is in error, or you want to debate the merits of my existence (sniffle), please contact inquiries@sailsjs.com

sgress454 commented 7 years ago

@mjasnikovs I'm unable to reproduce this. Would you mind running the Sails core Redis tests and seeing if you experience the same issue? To do so, check out the Sails master branch from Github:

git clone https://github.com/balderdashy/sails.git
npm install

Then start a Redis server on port 6380:

redis-server --port 6380

and run the full test suite with the TEST_REDIS_SESSION env var on:

set TEST_REDIS_SESSION=true
npm test

or to just run the Redis tests, if you have mocha installed globally:

set TEST_REDIS_SESSION=true
npm test mocha test/integration/middleware.session.redis.test.js
luislobo commented 7 years ago

@sgress454 I tried this with the latest master and had no errors

mjasnikovs commented 7 years ago

Tests do not work on windows, I suspect test string in packaged.json in not appropriate for windows env.

I removed all databases from the project, I have a problem only when "connect-redis" is inside session config file.

module.exports.session = {
    // Session secret is automatically generated when your new app is created
    // Replace at your own risk in production-- you will invalidate the cookies
    // of your users, forcing them to log in again.

    secret: '18677c5b37ee3ef046cba9c71d64cb9a',

    adapter: 'connect-redis',
    host: '127.0.0.1',
    port: 6379,
    // // ttl: <redis session TTL in seconds>,
    db: 1,
    // pass: <redis auth password>,
    // prefix: 'sess:'

    // Customize when built-in session support will be skipped.
    // (Useful for performance tuning; particularly to avoid wasting cycles on
    // session management when responding to simple requests for static assets,
    // like images or stylesheets.)

    // https://sailsjs.com/config/session

    // isSessionDisabled: function (req){
    //   return !!req.path.match(req._sails.LOOKS_LIKE_ASSET_RX);
    // },
}
mikermcneil commented 7 years ago

@mjasnikovs I saw something like this happen about 9 months ago in an early prerelease of Sails v1. In my case, it was with a command-line script that was managing the sails lifecycle.

(digging that up...)

OK so here are the old test instructions I used when reproducing it back then:

To test:

1. Create a new Sails app
2. `npm install connect-redis` and set `adapter: 'connect-redis'` in `config/session.js`
3. Start a local Redis server
4. `node -e "require('sails').load(function(err, _sails) {_sails.lower();});"`

Verify that the process doesn't hang and you're returned to the command prompt.

image

@sgress454 at that time, you mentioned the fix was: • https://github.com/balderdashy/sails/commit/80fb71b45fa7dab94f4c6054b6d365a9319150f7 • and also https://github.com/balderdashy/sails-hook-sockets/commit/b72db3f3ea0c4b9c3d35c93cd83862f8fc1a63bc

Got too many plates spinning right now to dig into this one further, but wanted to jot all that down while it was on my mind

sgress454 commented 7 years ago

@mjasnikovs

Tests do not work on windows, I suspect test string in packaged.json in not appropriate for windows env.

Your suspicions are correct -- you can run tests on Windows with npm run custom-tests.

mikermcneil commented 7 years ago

@sgress454 thanks for the reminder: https://github.com/balderdashy/sails/commit/11defad7a9693f552626be3fd8ce67f6f6897d3a

sailsbot commented 6 years ago

@mjasnikovs,@sailsbot,@sgress454,@luislobo,@mikermcneil: Hello, I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message and simply close the issue if possible. On the other hand, if you are still waiting on a patch, please post a comment to keep the thread alive (with any new information you can provide).

If no further activity occurs on this thread within the next 3 days, the issue will automatically be closed.

Thanks so much for your help!

shailesh-kanzariya commented 3 years ago

@sgress454 @mikermcneil I am facing this issue when running test with mocha and using adapter: 'connect-redis' in config/session.js. Redis connection remains open even after sails.lower(done); is run successfully in lifecycle.test.js and therefore mocha does not exit and have to terminate it forcefully with ctrl+c. My package.json has:

If I close the redis connection in mocha test file: after(() => {sails.config.session.client.quit(); }then mocha does not wait and returns normally otherwise have to terminate it with ctrl+c. Any idea when closing the redis connection will be taken care by sails.lower(done); ?

jspark-gigworks commented 3 years ago

My case is the same with @shailesh-kanzariya I wrote the information.