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

Ensure generate timeout is triggered when staleTimeout > ttl #178

Merged
merged 1 commit into from May 27, 2017

Conversation

@kanongil
Copy link
Member

kanongil commented Jan 23, 2017

This fixes a critical issue where a generate that never completes can cause get() calls to queue up indefinitely even though generateTimeout is specified.

This issue is especially critical in combination with pendingGenerateTimeout, which will never call respond() if there is a pending generate.

Copy link
Member

devinivy left a comment

Looks good to me.

@devinivy

This comment has been minimized.

Copy link
Member

devinivy commented Jan 23, 2017

The code looks good and seems to work as advertised, but upon further review I am still pondering the implications of this change.

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Jan 23, 2017

@devinivy What are your concerns?

@devinivy

This comment has been minimized.

Copy link
Member

devinivy commented Jan 23, 2017

I just wanted to make sure I fully understood what was going on, cause it requires a slightly subtle understanding of how the configuration options interact with each other. I thought about it more, and I think the fix is good 👍

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Jan 23, 2017

I have thought about it quite a bit, and I'm happy that you agree. It is a bit convoluted and somewhat complicated by a partially broken pendingGenerateTimeout implementation.

Points to ease comprehension:

  • Policy#get() will queue calls while a Policy#_generate() is processing for key.
  • If Policy#_generate() ever fails to call respond(), no responses will be triggered.
  • generateTimeout is supposed to act as a safeguard when generateFunc doesn't call the callback,
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Jan 26, 2017

I'll try to get to this early next week.

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Feb 10, 2017

@hueniverse I hate to poke but this fixes a rather serious (though hard to trigger) bug that effects any hapi server that uses catbox caching. Hope you will find some time to look at it soon.

@jgallen23

This comment has been minimized.

Copy link

jgallen23 commented Apr 11, 2017

We are running into this as well, if our api server goes down the get calls don't complete and just pile up, so our webserver's cpu spikes and will not go down unless we restart them.

@devinivy

This comment has been minimized.

Copy link
Member

devinivy commented Apr 11, 2017

Yes, this is definitely an outstanding bug– I still think the fix here is solid 👍

@hueniverse hueniverse self-assigned this May 27, 2017
@hueniverse hueniverse added the bug label May 27, 2017
@hueniverse hueniverse added this to the 7.1.4 milestone May 27, 2017
@hueniverse hueniverse merged commit ade34e3 into hapijs:master May 27, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hueniverse added a commit that referenced this pull request May 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.