Skip to content

Flush cache manager at command termination (fix #32) #38

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

Merged
merged 2 commits into from
Mar 15, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
29 changes: 28 additions & 1 deletion EventListener/InvalidationListener.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
use FOS\HttpCacheBundle\Configuration\InvalidatePath;
use FOS\HttpCacheBundle\Configuration\InvalidateRoute;
use FOS\HttpCacheBundle\Invalidator\InvalidatorCollection;
use Symfony\Component\Console\ConsoleEvents;
use Symfony\Component\Console\Event\ConsoleEvent;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\ExpressionLanguage\SyntaxError;
use Symfony\Component\ExpressionLanguage\ExpressionLanguage;
Expand Down Expand Up @@ -124,12 +126,37 @@ public function onKernelTerminate(PostResponseEvent $event)
return $this->cacheManager->flush();
}

/**
* Flush cache manager when kernel exception occurs
*/
public function onKernelException()
{
$this->cacheManager->flush();
Copy link
Contributor

Choose a reason for hiding this comment

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

i was wondering if we could return the number of requests we flushed in the CacheManager and use that to output a line on the console "Sent 5 invalidation requests to the caching proxy." - for the verbose mode.

Copy link
Member Author

Choose a reason for hiding this comment

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

This can be done, with some changes in the library. And there’s some ambiguity: does “5 requests” mean: 5 requests to each proxy server, or 5 requests in total?

Copy link
Contributor

Choose a reason for hiding this comment

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

i would mean to each proxy. making this more explicit makes sense yes. btw, i would only ouptut a message when there are > 0 requests, as otherwise we would output that line after every command run.

and yes, we would need to change on the library, but i think it would make sense. shall i do a PR on the library?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, please!

Copy link
Contributor

Choose a reason for hiding this comment

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

}

/**
* Flush cache manager when console terminates or errors
*/
public function onConsoleTerminate(ConsoleEvent $event)
{
$num = $this->cacheManager->flush();

if ($num > 0 && $event->getInput()->getOption('verbose')) {
$event->getOutput()->writeln(sprintf('Sent %d invalidation requests', $num));
}
}

/**
* {@inheritdoc}
*/
public static function getSubscribedEvents()
{
return array(KernelEvents::TERMINATE => 'onKernelTerminate');
return array(
KernelEvents::TERMINATE => 'onKernelTerminate',
Copy link
Contributor

Choose a reason for hiding this comment

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

note that onKernelTerminate contains

     if (!$response->isSuccessful()) {
         return $this->cacheManager->flush();
     }

not sure if this is still called after onKernelException, but if it is we would not need to listen to the exception but simple remove the skip flush code.

Copy link
Member Author

Choose a reason for hiding this comment

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

The kernel.terminate event is not thrown when an exception occurs, so let’s keep them separate.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay. but the check for response->isSuccessful() could still be removed i think. not sure when it would even happen - if a caught exception is handled and a controller decides to return a response with an error code?

Copy link
Member Author

Choose a reason for hiding this comment

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

In case someone returns an error response (404, for instance) from a controller whether an exception occurred or not:

return new Response('Ooops', 404);

Do we want to accommodate this case, or just assume that when onKernelTerminate is reached nothing bad happened?

Copy link
Contributor

Choose a reason for hiding this comment

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

i would try to flush the queue in as many situations as we ever can. die/fatal errors will prevent the flush, but otherwise flush everywhere we can. in the worst case, we just unnecessarily remove a cache entry that would not have had to be removed. that is a very harmless case, compared to not invalidate.

if anything, we could add a clear method to undo scheduled invalidations by emptying the queue. then somebody with a very specific use case could handle that.

hm. what happens with the tag based invalidation when the symfony firewall prevents access to a url / controller? what if the controller itself has a failed security check? that might be the case where we do not want to invalidate on a 403. it would need quite some understanding of the app to do this attack, but it could be rather nasty. maybe do not handle the tags on 403 errors? explicit invalidations should still be flushed - a security check should be done at the beginning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't this currently the case? If the response is unsuccessful (includes 403), any annotation/config invalidations are not added to the queue, but the queue is still flushed for manual/explicit invalidations.

Copy link
Contributor

Choose a reason for hiding this comment

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

seems i was confused. indeed it is.

KernelEvents::EXCEPTION => 'onKernelException',
ConsoleEvents::TERMINATE => 'onConsoleTerminate',
ConsoleEvents::EXCEPTION => 'onConsoleTerminate'
);
}

/**
Expand Down
22 changes: 7 additions & 15 deletions Resources/doc/cache-manager.md
Original file line number Diff line number Diff line change
Expand Up @@ -130,20 +130,12 @@ Flushing
--------

Internally, the invalidation requests are queued and only sent out to your HTTP
proxy when the manager is flushed. During HTTP requests, the manager is flushed
automatically. If you want to invalidate objects outside request context, for
instance from the command-line, you need to flush the cache manager manually
(until https://github.com/ddeboer/FOSHttpCacheBundle/issues/32 is done):
proxy when the manager is flushed. The manager is flushed automatically at the
right moment:

```php
$cacheManager
->invalidateRoute(...)
->invalidatePath(...)
->flush();
```

The performance impact of sending invalidation requests is kept to a minimum by:
* when handling a HTTP request, after the response has been sent to the client
(Symfony’s [kernel.terminate event](http://symfony.com/doc/current/components/http_kernel/introduction.html#the-kernel-terminate-event))
* when running a console command, after the command has finished (Symfony’s
[console.terminate event](http://symfony.com/doc/current/components/console/events.html#the-consoleevents-terminate-event)).

* flushing the cache manager only after the response by your controller has been sent to the client’s browser
(during Symfony’s [kernel.terminate event](http://symfony.com/doc/current/components/http_kernel/introduction.html#the-kernel-terminate-event)).
* sending all invalidation requests in parallel.
You can also [flush the cache manager manually](https://github.com/FriendsOfSymfony/FOSHttpCache/blob/master/doc/cache-invalidator.md#flushing).
Copy link
Contributor

Choose a reason for hiding this comment

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

should we mention here that the queue is emptied after flushing, and thus you can repeatedly call flush in a sane way?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, that is good enough

21 changes: 12 additions & 9 deletions Tests/EventListener/InvalidationListenerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,17 @@

namespace FOS\HttpCacheBundle\Tests\EventListener;

use Doctrine\Common\Annotations\AnnotationReader;
use FOS\HttpCache\Invalidation\Method\BanInterface;
use FOS\HttpCache\Invalidation\Method\PurgeInterface;
use FOS\HttpCacheBundle\Configuration\Invalidate;
use FOS\HttpCacheBundle\Configuration\InvalidatePath;
use FOS\HttpCacheBundle\Configuration\InvalidateRoute;
use FOS\HttpCacheBundle\EventListener\InvalidationListener;
use FOS\HttpCacheBundle\Invalidator\Invalidator;
use FOS\HttpCacheBundle\Invalidator\InvalidatorCollection;
use FOS\HttpCacheBundle\Tests\EventListener\Fixture\FooControllerTagAtMethod;
use Sensio\Bundle\FrameworkExtraBundle\EventListener\ControllerListener;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\Event\FilterControllerEvent;
use Symfony\Component\HttpKernel\Event\PostResponseEvent;
use \Mockery;
use Symfony\Component\HttpKernel\HttpKernelInterface;
use Symfony\Component\Routing\Route;
use Symfony\Component\Routing\RouteCollection;
use \Mockery;

class InvalidationListenerTest extends \PHPUnit_Framework_TestCase
{
Expand Down Expand Up @@ -133,6 +125,17 @@ public function testInvalidateRoute()
$this->getListener()->onKernelTerminate($event);
}

public function testOnConsoleTerminate()
{
$this->cacheManager->shouldReceive('flush')->once()->andReturn(2);

$event = \Mockery::mock('\Symfony\Component\Console\Event\ConsoleEvent');
$event->shouldReceive('getInput->getOption')->with('verbose')->andReturn(true);
$event->shouldReceive('getOutput->writeln')->with('Sent 2 invalidation requests')->once();

$this->getListener()->onConsoleTerminate($event);
}

protected function getEvent(Request $request, Response $response = null)
{
return new PostResponseEvent(
Expand Down
1 change: 1 addition & 0 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
"mockery/mockery": "0.8.*",
"monolog/monolog": "*",
"sensio/framework-extra-bundle": "~2.3",
"symfony/console": "~2.3",
"symfony/security": "~2.3",
"symfony/expression-language": "~2.4"
},
Expand Down