-
Notifications
You must be signed in to change notification settings - Fork 84
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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(); | ||
} | ||
|
||
/** | ||
* 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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note that onKernelTerminate contains
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. if anything, we could add a 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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' | ||
); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay, that is good enough |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FriendsOfSymfony/FOSHttpCache#47