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

Proposal: Stale while revalidate - prevent multiple generate calls #149

Closed
nathanmesserbbc opened this issue Nov 11, 2015 · 8 comments
Closed
Assignees
Labels
Milestone

Comments

@nathanmesserbbc
Copy link
Contributor

@nathanmesserbbc nathanmesserbbc commented Nov 11, 2015

In a situation where a fast response is desireable, and a generate function is slow, multiple unnecessary calls to the generate function are currently made.

Assume there is already a stale value for a key in the cache.

Multiple requests come in.

For the duration of the stale timeout, only a single call to the generate function is made.

However, once the stale timeout expires, and those pending requests are responded to, there is an outstanding call to the generate function in progress, but further calls will lead to another unnecessary call to the generate function.

I'm suggesting a new option, pendingTimeout, which if set, would have to be greater than stateTimeout.

If set, when the staleTimeout is reached, the pending responses would be sent, and the callbacks removed from the array, but the key would remain in the pending list, meaning that additional calls would wait for the same call to the generate function to complete.

When the pendingTimeout was reached, THEN the key would be deleted from the pending list.

This would allow a short staleTimeout (say 200ms) for a long running generate function (say 5s), with only a single call to the generate function, as opposed to the current situation where that setup could generate up to 25 calls to the generate function.

If this seems reasonable, I'm happy to implement and submit a pull request.

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Nov 11, 2015

This feels complicated and if your staleTimeout is so short, you are problem using the wrong tool. catbox was created for caching of HTTP related activities with normal timeouts in seconds (really minutes and hours). What are you doing that you need such short timeouts?

@hueniverse hueniverse added the request label Nov 11, 2015
@nathanmesserbbc

This comment has been minimized.

Copy link
Contributor Author

@nathanmesserbbc nathanmesserbbc commented Nov 11, 2015

We are using it for caching http related activities.

However, we're using it (quite successfully generally) to deliver stale-while-revalidate functionality.

So we have an http call that can take around 10 seconds.
We'd like to deliver a response much faster than that, and are quite happy to serve a stale response for a short while.

So by setting a staleTimeout of 250ms, we get a quick (but stale) response if there is one in the cache, and in the meantime the value is updated in the background once the http request completes.

The only issue is that when under load we generate a lot of unnecessary http calls while the initial long running call is being processed.

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Nov 11, 2015

Give it a shot. I can't promise I'll like it :-)

@nathanmesserbbc

This comment has been minimized.

Copy link
Contributor Author

@nathanmesserbbc nathanmesserbbc commented Nov 12, 2015

Just out of interest, would you usually expect long staleTimeout (as opposed to staleIn) values?

We use long staleIn values (minutes to hours depending on use case), but very short staleTimeout values (time to wait for the generateFunction to execute before returning a stale response).

I'm curious as to the use case/benefit of a long staleTimeout (rather than staleIn value) of minutes or hours.

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Nov 13, 2015

Your use case makes sense. No worries.

@hueniverse hueniverse closed this Nov 13, 2015
@hueniverse hueniverse reopened this Nov 13, 2015
@primitive-type

This comment has been minimized.

Copy link

@primitive-type primitive-type commented Dec 10, 2015

@nathanmesserbbc I have exactly this use case as well. Looking forward to what you come up with here. Thanks!

@nathanmesserbbc

This comment has been minimized.

Copy link
Contributor Author

@nathanmesserbbc nathanmesserbbc commented Dec 11, 2015

@primitive-type I've got a change that implements that ready to go here:

https://github.com/nathanmesserbbc/catbox/tree/pending_generate_timeout

I'm just testing it out to make sure it works as expected before submitting a PR.

@hueniverse not sure if you want to wait for a PR, or take a look now and see if you think the approach fits.

@nathanmesserbbc

This comment has been minimized.

Copy link
Contributor Author

@nathanmesserbbc nathanmesserbbc commented Dec 11, 2015

@hueniverse hueniverse added this to the 7.1.0 milestone Jan 4, 2016
@hueniverse hueniverse self-assigned this Jan 4, 2016
@hueniverse hueniverse closed this Jan 4, 2016
@Marsup Marsup added feature and removed request labels Sep 21, 2019
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.