Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option to skip exception on cache error #83

Merged
merged 25 commits into from Oct 1, 2015
Merged

Conversation

@tswayne
Copy link

tswayne commented Sep 22, 2015

No description provided.

@mark-bradshaw mark-bradshaw self-assigned this Sep 23, 2015
@mark-bradshaw mark-bradshaw added this to the 4.3.0 milestone Sep 23, 2015
API.md Outdated
@@ -7,6 +7,7 @@
- `name` - determines the name of the cookie used to store session information. Defaults to _session_.
- `maxCookieSize` - maximum cookie size before using server-side storage. Defaults to 1K. Set to zero to always use server-side storage.
- `storeBlank` - determines whether to store empty session before they've been modified. Defaults to _true_.
- `errorOnCacheDisconnect` - will cause yar to throw exception if trying to persist to cache when cache is unavailable. Setting to false will allow application using yar to run uninterrupted if cache is not ready (however sessions will not be saving). Defaults to _true_.

This comment has been minimized.

Copy link
@mark-bradshaw

mark-bradshaw Sep 23, 2015

Contributor

I think I'd prefer this be called errorOnCacheNotReady. I think that's a bit clearer.

This comment has been minimized.

Copy link
@tswayne

tswayne Sep 23, 2015

Author

I agree. Done.

request.session.id) {
var requestSessionSet = request.session && request.session.id;
var loadFromCache = settings.errorOnCacheNotReady || cache.isReady();
if ( requestSessionSet && loadFromCache) {

This comment has been minimized.

Copy link
@mark-bradshaw

mark-bradshaw Sep 23, 2015

Contributor

extra space here

if (!request.session ||
(!request.session._isModified && !request.session._isLazy)) {
skipCacheStorage ||

This comment has been minimized.

Copy link
@mark-bradshaw

mark-bradshaw Sep 23, 2015

Contributor

I'm thinking this may not be placed correctly. Seems to me we still want to go through the prepare() and cookie() functions. We just want to skip the storage() function. Thoughts?

This comment has been minimized.

Copy link
@tswayne

tswayne Sep 23, 2015

Author

Good point. To that point, the check on preResponse is a tad too early too. I will move that check to the line that checks if the cookie returned session data and add a test for that.

This comment has been minimized.

Copy link
@tswayne

tswayne Sep 23, 2015

Author

Done, the implementation is a bit wordy, so I'll refactor tomorrow when I pull out sinon.


return reply();
});
return reply();

This comment has been minimized.

Copy link
@mark-bradshaw

mark-bradshaw Sep 23, 2015

Contributor

Thanks for catching this!

var cookie = header[0].match(/(session=[^\x00-\x20\"\,\;\\\x7F]*)/);

expect(res.statusCode).to.equal(200);
expect(handlerSpy.calledOnce).to.equal(true);

This comment has been minimized.

Copy link
@mark-bradshaw

mark-bradshaw Sep 23, 2015

Contributor

As a preference I'd prefer to avoid adding sinon to the mix. For better or worse it injects a new methodology of testing than what has been used here. It's not wrong, but I'd like to keep things consistent.

So, for instance, are you really trying to determine whether the handler got called once, or at all? If "at all", how about just return a value on line 614 and test for it in the return result? That's basically what the older tests have been doing.

This comment has been minimized.

Copy link
@tswayne

tswayne Sep 24, 2015

Author

I have removed sinon and just stubbed methods where needed.

expect(res.statusCode).to.equal(200);
expect(handlerSpy.calledOnce).to.equal(true);

cache.prototype.set.restore();

This comment has been minimized.

Copy link
@mark-bradshaw

mark-bradshaw Sep 23, 2015

Contributor

And for this, can we just add a public method/object on the failing cache mock object that allows us to control how set returns? Again, just to avoid adding another dependency and style to the project.

@mark-bradshaw

This comment has been minimized.

Copy link
Contributor

mark-bradshaw commented Sep 23, 2015

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.

tswayne added 2 commits Sep 23, 2015
@mark-bradshaw

This comment has been minimized.

Copy link

mark-bradshaw commented on lib/index.js in 884afa4 Sep 23, 2015

Yeah, I think that's better.

tswayne
@mark-bradshaw

This comment has been minimized.

Copy link
Contributor

mark-bradshaw commented Sep 23, 2015

Thanks. It's looking good.

@@ -7,6 +7,7 @@
- `name` - determines the name of the cookie used to store session information. Defaults to _session_.
- `maxCookieSize` - maximum cookie size before using server-side storage. Defaults to 1K. Set to zero to always use server-side storage.
- `storeBlank` - determines whether to store empty session before they've been modified. Defaults to _true_.
- `errorOnCacheNotReady` - will cause yar to throw exception if trying to persist to cache when cache is unavailable. Setting to false will allow application using yar to run uninterrupted if cache is not ready (however sessions will not be saving). Defaults to _true_.

This comment has been minimized.

Copy link
@hueniverse

hueniverse Sep 24, 2015

Member

Change to "Defaults to false"

This comment has been minimized.

Copy link
@tswayne

tswayne Sep 24, 2015

Author

Ok, I flipped the default and adjusted the tests.

This comment has been minimized.

Copy link
@mark-bradshaw

mark-bradshaw Sep 28, 2015

Contributor

Why flip this to false? The current behavior matches setting this to true. I don't want to break expectations.

if (request.session &&
request.session.id) {

if ( request.session && request.session.id ) {

This comment has been minimized.

Copy link
@hueniverse

hueniverse Sep 24, 2015

Member

Don't change style.

This comment has been minimized.

Copy link
@tswayne

tswayne Sep 24, 2015

Author

Put back.

@@ -191,7 +195,7 @@ exports.register = function (server, options, next) {
server.ext('onPreResponse', function (request, reply) {

if (!request.session ||
(!request.session._isModified && !request.session._isLazy)) {
(!request.session._isModified && !request.session._isLazy)) {

This comment has been minimized.

Copy link
@hueniverse

hueniverse Sep 24, 2015

Member

Again, why change spacing?

This comment has been minimized.

Copy link
@tswayne

tswayne Sep 24, 2015

Author

Put back.

@@ -248,15 +252,20 @@ exports.register = function (server, options, next) {

var storage = function () {

reply.state(settings.name, { id: request.session.id });
cache.set(request.session.id, request.session._store, 0, function (err) {
if (settings.errorOnCacheNotReady || cache.isReady()) {

This comment has been minimized.

Copy link
@hueniverse

hueniverse Sep 24, 2015

Member

More readable to handle the else first and return.

This comment has been minimized.

Copy link
@tswayne
tswayne added 2 commits Sep 24, 2015
tswayne
tswayne
tswayne added 4 commits Sep 24, 2015
tswayne
tswayne
tswayne
@@ -60,6 +61,11 @@ exports.register = function (server, options, next) {
request.session.id) {

request.session._isModified = false;
if (!settings.errorOnCacheNotReady && !cache.isReady()) {
if (!request.session._store) {

This comment has been minimized.

Copy link
@hueniverse

hueniverse Sep 24, 2015

Member

The two if statement should be combined into one.

tswayne
@mark-bradshaw

This comment has been minimized.

Copy link
Contributor

mark-bradshaw commented Sep 29, 2015

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.

API.md Outdated
@@ -7,6 +7,7 @@
- `name` - determines the name of the cookie used to store session information. Defaults to _session_.
- `maxCookieSize` - maximum cookie size before using server-side storage. Defaults to 1K. Set to zero to always use server-side storage.
- `storeBlank` - determines whether to store empty session before they've been modified. Defaults to _true_.
- `errorOnCacheNotReady` - will cause yar to throw exception if trying to persist to cache when cache is unavailable. Setting to false will allow application using yar to run uninterrupted if cache is not ready (however sessions will not be saving). Defaults to _false_.

This comment has been minimized.

Copy link
@hueniverse

hueniverse Sep 29, 2015

Member

Where is it throwing an exception? If this is a configurable option, it should explicitly throw or return an error rather than throw because the code fails to protect against a condition.

This comment has been minimized.

Copy link
@mark-bradshaw

mark-bradshaw Sep 29, 2015

Contributor

Tyler's email described all this. The current behavior of yar throws when the cache isn't available. This option allows yar to continue on without the cache, if desired, and I think that's a useful option. The current behavior of yar is equivalent to errorOnCacheNotReady = true, which is why I want that to default to true. If you choose to set it to false to avoid the thrown errors, then that's your choice.

This comment has been minimized.

Copy link
@hueniverse

hueniverse Sep 29, 2015

Member

Where is it throwing?

This comment has been minimized.

Copy link
@tswayne

tswayne Sep 30, 2015

Author

Error is returned from cache set and get, line 72 and 260 and returned to reply where hapi returns 500. This option skips those steps to avoid certain exception if the cache is not ready. When that happens however, the session data does not persist, which is why it shouldn't be default functionality.

tswayne added 5 commits Sep 30, 2015
tswayne
* Add coverage
tswayne
tswayne
@tswayne

This comment has been minimized.

Copy link
Author

tswayne commented Sep 30, 2015

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

@mark-bradshaw

This comment has been minimized.

Copy link
Contributor

mark-bradshaw commented Sep 30, 2015

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

This comment has been minimized.

Copy link
Member

hueniverse commented Sep 30, 2015

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

This comment has been minimized.

Copy link
Contributor

mark-bradshaw commented Oct 1, 2015

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

This comment has been minimized.

Copy link
Member

hueniverse commented Oct 1, 2015

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

This comment has been minimized.

Copy link
Contributor

mark-bradshaw commented Oct 1, 2015

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
@tswayne

This comment has been minimized.

Copy link
Author

tswayne commented Oct 1, 2015

@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

This comment has been minimized.

Copy link
Member

hueniverse commented Oct 1, 2015

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

This comment has been minimized.

Copy link
Contributor

mark-bradshaw commented Oct 1, 2015

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
#83 (comment).

mark-bradshaw added a commit that referenced this pull request Oct 1, 2015
Option to skip exception on cache error
@mark-bradshaw mark-bradshaw merged commit 09cafe8 into hapijs:master Oct 1, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.