Redis down fix

[Chuwiey]

This is related to the issue I filed earlier today: #2276

If I set the session and socket stores (or one of them) to use Redis as the adapter, and Redis goes down (for whatever reason), sails has an un-handled exception and crashes. It's actually really easy to fix this and make Sails send a 500 while Redis is down, and then when it comes back up, just re-connect.

Steps to reproduce current bug:

1. Make sure you have configured Redis as the session store (and the socket store)
2. sails lift (while redis is running)
3. go to a page on your app, or any route that works - it works
4. now shutdown redis
5. sails has crashed

I ran the tests, and everything seems to work.

Thanks!

[Ariel-Rodriguez]

Awesome! simple solution for a big blocker.

Hopefully @sgress454 agrees on merging this 👍 . Thanks!

[sgress454]

After looking into this, it seems to me that the only part that causes Sails to crash is the socket store. Your changes to loadSocketIO.js definitely fix that. If you're only using Redis for sessions, Sails won't crash when Redis goes down--it'll just show:

Unable to parse HTTP body- error occurred :: 'Redis connection gone from close event.'

This is an ugly and misleading message (it has nothing to do with parsing an HTTP body), but that's a separate problem that deserves a more comprehensive solution. So, I'd accept a modified version of this PR without the changes to the http hook and the sails.config.session.state code.

[Chuwiey]

I have no problem with reverting that part, however, here's my justification for it:
The http parse error appears anyway, once you catch the redis 'end' or 'error' events. Sockets and sessions share the same redis connections when they are both set to use redis. As you said, the http parse error is misleading,from the research I did, it originates in the connect redis package and has to do with event handling on their side. Adding the express middleware to catch requests that come in when redis is down and return a 500 is nicer than displaying the http parse error. I couldn't figure out how to show the 500 page though, since the response object doesn't have the view property that early in the Init.

Anyway, I'm happy to remove that portion, but I feel that while waiting for redis-connect to fix the event handling, it would be nicer to show users a 500 instead...

What do you think?
Thanks!

[sgress454]

Yes, I agree the error is misleading and needs a fix, but the fix you're proposing will only handle cases where the origin of the problem is a missing Redis session. What we really need is a generic error handler middleware; right now the handleBodyParserError middleware is performing that task, but it expects that any time it is called it's because there was a problem parsing the HTTP body, which is clearly not the case. The upshot is that you've identified a separate issue that we weren't aware of before, which will I'm sure lead to a future patch!

[Chuwiey]

@sgress454 removed the middleware, I also touched up the error messaging - let me know if you have any notes on that.

As a sideline, if you can give some direction as to what you'd like a generic error handler middleware to look like, I could work on that as the http parse error is blocking something we're doing with our project.

Thanks

[sgress454]

@Chuwiey I'll give it some thought. How is the parse error blocking you, and was it somehow solved by that additional middleware? The current middleware sends back a 400 error with that "http parse error" string, which you can check for as easily as checking for a 500...

[Chuwiey]

@sgress454 The issue was the amount of time it took for the app to respond with the 400; I deal with a rather high amount of requests coming in, and it seems that it takes a couple of seconds for each request that fails on http parse to come back. The middleware I created basically responded immediately because it didn't try the redis client first.

Also, I wanted to present users with a nice page talking about an error (whether 500 or 400 doesn't matter). Maybe I'll use some sort of catch all at the app level, we'll see.

Thanks for merging.