Skip to content

Refactor stale-while-revalidate code in HttpCache, add a (first?) test for it #22043

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Refactor stale-while-revalidate code in HttpCache, add a (first?) test for it #22043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d880638
Improve stale-while-revalidate code in HttpCache, add a (first?!?) te…
mpdude Mar 17, 2017
24f605a
fabbot.io
mpdude Mar 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 63 additions & 36 deletions src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,49 +549,39 @@ protected function lock(Request $request, Response $entry)
// try to acquire a lock to call the backend
$lock = $this->store->lock($request);

if (true === $lock) {
// we have the lock, call the backend
return false;
}

// there is already another process calling the backend
if (true !== $lock) {
// check if we can serve the stale entry
if (null === $age = $entry->headers->getCacheControlDirective('stale-while-revalidate')) {
$age = $this->options['stale_while_revalidate'];
}

if (abs($entry->getTtl()) < $age) {
$this->record($request, 'stale-while-revalidate');
// May we serve a stale response?
if ($this->mayServeStaleWhileRevalidate($entry)) {
$this->record($request, 'stale-while-revalidate');

// server the stale response while there is a revalidation
return true;
}

// wait for the lock to be released
$wait = 0;
while ($this->store->isLocked($request) && $wait < 5000000) {
usleep(50000);
$wait += 50000;
}
return true;
}

if ($wait < 5000000) {
// replace the current entry with the fresh one
$new = $this->lookup($request);
$entry->headers = $new->headers;
$entry->setContent($new->getContent());
$entry->setStatusCode($new->getStatusCode());
$entry->setProtocolVersion($new->getProtocolVersion());
foreach ($new->headers->getCookies() as $cookie) {
$entry->headers->setCookie($cookie);
}
} else {
// backend is slow as hell, send a 503 response (to avoid the dog pile effect)
$entry->setStatusCode(503);
$entry->setContent('503 Service Unavailable');
$entry->headers->set('Retry-After', 10);
// wait for the lock to be released
if ($this->waitForLock($request)) {
// replace the current entry with the fresh one
$new = $this->lookup($request);
$entry->headers = $new->headers;
$entry->setContent($new->getContent());
$entry->setStatusCode($new->getStatusCode());
$entry->setProtocolVersion($new->getProtocolVersion());
foreach ($new->headers->getCookies() as $cookie) {
$entry->headers->setCookie($cookie);
}

return true;
} else {
// backend is slow as hell, send a 503 response (to avoid the dog pile effect)
$entry->setStatusCode(503);
$entry->setContent('503 Service Unavailable');
$entry->headers->set('Retry-After', 10);
}

// we have the lock, call the backend
return false;
return true;
}

/**
Expand Down Expand Up @@ -710,4 +700,41 @@ private function record(Request $request, $event)
}
$this->traces[$request->getMethod().' '.$path][] = $event;
}

/**
* Checks whether the given (cached) response may be served as "stale" when a revalidation
* is currently in progress.
*
* @param Response $entry
*
* @return bool True when the stale response may be served, false otherwise.
*/
private function mayServeStaleWhileRevalidate(Response $entry)
{
$timeout = $entry->headers->getCacheControlDirective('stale-while-revalidate');

if ($timeout === null) {
$timeout = $this->options['stale_while_revalidate'];
}

return abs($entry->getTtl()) < $timeout;
}

/**
* Waits for the store to release a locked entry.
*
* @param Request $request The request to wait for
*
* @return bool True if the lock was released before the internal timeout was hit; false if the wait timeout was exceeded.
*/
private function waitForLock(Request $request)
{
$wait = 0;
while ($this->store->isLocked($request) && $wait < 5000000) {
usleep(50000);
$wait += 50000;
}

return $wait < 5000000;
}
}
36 changes: 36 additions & 0 deletions src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,42 @@ public function testHitsCachedResponseWithMaxAgeDirective()
$this->assertEquals('Hello World', $this->response->getContent());
}

public function testDegradationWhenCacheLocked()
{
$this->cacheConfig['stale_while_revalidate'] = 10;

// The prescence of Last-Modified makes this cacheable (because Response::isValidateable() then).
$this->setNextResponse(200, array('Cache-Control' => 'public, s-maxage=5', 'Last-Modified' => 'some while ago'), 'Old response');
$this->request('GET', '/'); // warm the cache

// Now, lock the cache
$concurrentRequest = Request::create('/', 'GET');
$this->store->lock($concurrentRequest);

/*
* After 10s, the cached response has become stale. Yet, we're still within the "stale_while_revalidate"
* timeout so we may serve the stale response.
*/
sleep(10);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sleeping for 20 seconds in total in this test is unacceptable. Is it possible to sleep for less, or don't sleep at all?

Copy link
Member

@javiereguiluz javiereguiluz Mar 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With PHPUnitBridge clock mocking, time passes instantly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't @group time-sensitive / clock mocking solve this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does indeed. I only checked the method annotations (none). Indeed time-sensitive is set on the test class 👍


$this->request('GET', '/');
$this->assertHttpKernelIsNotCalled();
$this->assertEquals(200, $this->response->getStatusCode());
$this->assertTraceContains('stale-while-revalidate');
$this->assertEquals('Old response', $this->response->getContent());

/*
* Another 10s later, stale_while_revalidate is over. Resort to serving the old response, but
* do so with a "server unavailable" message.
*/
sleep(10);

$this->request('GET', '/');
$this->assertHttpKernelIsNotCalled();
$this->assertEquals(503, $this->response->getStatusCode());
$this->assertEquals('Old response', $this->response->getContent());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dunno why, but this test breaks the HttpKernel test suite on windows (see appveyor https://ci.appveyor.com/project/fabpot/symfony/build/1.0.20167#L841), removing the test fixes it.


public function testHitsCachedResponseWithSMaxAgeDirective()
{
$time = \DateTime::createFromFormat('U', time() - 5);
Expand Down
4 changes: 4 additions & 0 deletions src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ class HttpCacheTestCase extends TestCase
protected $responses;
protected $catch;
protected $esi;

/**
* @var Store
*/
protected $store;

protected function setUp()
Expand Down