search

hapijs / yar

A hapi session manager
Other
133 stars 59 forks source link

Option to skip exception on cache error #83

Closed tswayne closed 9 years ago

mark-bradshaw commented 9 years ago

That's all I have for now. I'm in favor of the change. I think it's valuable. If we can just pivot a few items then we'll get it in.

mark-bradshaw commented 9 years ago

Thanks. It's looking good.

mark-bradshaw commented 9 years ago

Hey Tyler, sorry for the back and forth and some confusion on the defaults. I would like you to switch the default on errorOnCacheNotReady back to true, and then we'll be ready to merge.

tswayne commented 9 years ago

I have flipped the default back and cleaned up some test descriptions, so this should be ready to go.

mark-bradshaw commented 9 years ago

Thanks Tyler. I'm going to make one more pass through to digest the final set of changes since we've gone back and forth a bit, and if all's good I'll merge.

hueniverse commented 9 years ago

This is an incomplete solution. We need to handle the cache errors regardless of what this option is set to and to handle them correctly. This doesn't actually does what it says but prevent errors from happening.

mark-bradshaw commented 9 years ago

I disagree. I think the option is doing exactly what it says. If it's set to true, and the cache isn't ready, an error is thrown. That's exactly what it describes. If you set this to false, you are choosing to ignore the error. I don't see the problem. What do you envision as the correct way to handle the errors?

hueniverse commented 9 years ago

It should check if the cache is ready. If it is not, and the option is not set to false, raise an actual error that clearly states the issue. What this is doing is causing a cache issue to fail elsewhere.

mark-bradshaw commented 9 years ago

So if the option errorOnCacheNotReady is set to false, it should error? That seems backwards, no? I can get behind raising a more specific error. That seems reasonable. But if you don't care about the cache server availability then you should be able to ignore this condition. Not every site will have a dedicated ops team that can jump on issues at any time of day or night. Tyler's patch is a valuable option, so I'll allow it.

@tswayne if you wouldn't mind one final change, if the yar is going to ignore the error, can you please at least do a request.log on it so that it can be caught and acted on. That should serve all our purposes. You can keep your servers up and running, but we can make sure the error is getting reported so it can be acted on.

tswayne commented 9 years ago

@hueniverse if hapi allowed us to catch that specific error and continue on through the request lifecycle (instead of returning 500 response), I would definitely agree that that would be the way to implement this. Since we have no way to catch the error (no matter how specific it is), handle it (log and NOT return error), and move on, we need to skip cache persistence entirely so we can avoid 500 responses on every request. This implementation allows users to let yar know that they do not want sessions to crash the application if cache is not available, and I agree with @mark-bradshaw that it is a valuable request. Additionally "What this is doing is causing a cache issue to fail elsewhere" is why this flag defaults to true by default. If the user explicitly sets that flag to false they are acknowledging that their sessions will not store or load from cache while their cache is not ready.

@mark-bradshaw I have added request.log

hueniverse commented 9 years ago

If the new option is set to false, it will not error. My point is that this solution is a hack, not a proper error handling solution. The problem isn't the new behavior its adding but the existing one it is trying to bypass. We need to first handle the existing error of cache unavailable and surface that better, then we can add the option to ignore it. I am not arguing against the current logic, only that it's an incomplete implementation, now that this issue was raised (which is really a bug).

mark-bradshaw commented 9 years ago

Noted.

On Thu, Oct 1, 2015 at 11:10 AM Eran Hammer notifications@github.com wrote:

If the new option is set to false, it will not error. My point is that this solution is a hack, not a proper error handling solution. The problem isn't the new behavior its adding but the existing one it is trying to bypass. We need to first handle the existing error of cache unavailable and surface that better, then we can add the option to ignore it. I am not arguing against the current logic, only that it's an incomplete implementation, now that this issue was raised (which is really a bug).

— Reply to this email directly or view it on GitHub https://github.com/hapijs/yar/pull/83#issuecomment-144774419.

lock[bot] commented 4 years ago

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.