[HttpKernel] Add Kernel::terminate() and HttpKernel::terminate() for post-response logic #2791
Conversation
| * | ||
| * Should be called before shutdown, but after sending the response | ||
| */ | ||
| function terminate(); |
There was a problem hiding this comment.
HttpKernelInterface was marked as stable so this is a BC break (simple way to see it: Silex needs to be updated after this change)
There was a problem hiding this comment.
I realize that, but I think we need to decide if this is needed first, and then how to deal with BC later.
There was a problem hiding this comment.
I think we should try to keep the HttpKernelInterface as simple as possible
There was a problem hiding this comment.
We could dispatch directly from the Kernel to avoid adding it to the interface, but IMO it belongs in the HttpKernel.
There was a problem hiding this comment.
it cannot be dispatch from the Kernel directly as it would stay before $response->send() if it is done inside handle :)
There was a problem hiding this comment.
that's the exact problem i stumbled upon ... ;-)
There was a problem hiding this comment.
It can be dispatched in the Kernel just fine with this, as I said the call to ->terminate has to be added in the front controller anyway, that does not change.
public function terminate()
{
$this->container->get('event_dispatcher')->dispatch(...);
}There was a problem hiding this comment.
nothing enforces having a dispatcher with this id. This is the name used by FrameworkBundle and we are in the component here.
Btw, nothing even forces to create a public service for the event dispatcher used by HttpKernel. It could be a private service that would then not be available through get at all (note that such a case simply implies that all listeners for the kernel events are registered through the DIC and that you don't use this dispatcher elsewhere).
There was a problem hiding this comment.
This should be irrelevant (otherwise, we must change the Kernel, too). Take a look at Kernel#181 - this enforces having a service with the id http_kernel which in case returns a HttpKernelInterface. This service is defined in the FrameworkBundle.
I think we could copycat this to retrieve the event dispatcher. An alternative is introducing an abstract method getEventDispatcher which must be implemented in the AppKernel class - in this case, the user is forced to provide a event dispatcher (which in case can be retrieved by the container) - or, the default implementation relies on container->get('event_dispatcher') and the user is free to overload this method.
I'm working on removing the BC breaks and introduce standalone classes/interfaces which could work like discussed.
There was a problem hiding this comment.
well, hardcoding the HttpKernel id does not seems clean either. But this is not a reason to add another one.
An btw, you could even implement all this without any event dispatcher if you change the implementation of HttpKernel for another one...
|
We discussed it on IRC and I suggested a way to avoid the BC break of the interface: adding a new interface ( For Symfony2 users it will mean they have to change their front controller to benefit from the new event of course, but this is easy to do. Btw, Silex can then be able to use it without any change for the end users as it can be done inside |
|
@Seldaek: I opened a pull request so that the discussion on IRC is fulfilled and no BC breaks exist: https://github.com/Seldaek/symfony/pull/1/files |
[HttpKernel] Add Kernel::terminate() and HttpKernel::terminate() for post-response logic (without the BC break)
|
Any real-world use case for this? |
|
Doing slow stuff after the user got his response back without having to implement a message queue. I believe @pminnieur wanted to use it to send logs to loggly? |
|
Its a good practice to defer code execution without the introduction of a new software layer (like gearman, amqp, whatever tools people use to defer code execution) which may be way too much just for the goal of having fast responses, whatever my code does. My real world use case which made me miss this feature the first time:
This means I have to create The other use case was loggly, yes. Sending logging data over the wire before the response is send doesn't make sense in my eyes, so it could be deferred after the response is send (this especially sucks if loggly fails and i get a 500 --the frontend/public user is not interested in a working logging facility, he wants his responses). |
|
This would help significantly, but the real problem, that your process is busy and unavailable for the next request, is still there. |
|
I think this is the wrong solution for a real problem. Saying "Its a good practice to defer code execution without the introduction of a new software layer" is just wrong. It is definitely a good practice to defer code execution, but you should use the right tool for the job. I'm -1. |
|
It should just give a possibility to put unimportant but heavy lifting code behind the send request with ease. With little effort people could benefit from the usage of It should not simulate node.js ;-) I agree that the real problem is not solved, but small problems could be solved easily. I personally don't want to setup RabbitMQ or whatever, maintain my crontab or any other software that may allow me to defer code execution. |
|
@fabpot: one could say that on shared hostings it is still useful because they generally don't give you gearman or *MQs. Anyway I think it'd be nice to really complete the HttpKernel event cycle. |
|
not only on shared hostings, sometimes teams/projects just don't have the resources or knowledge or time to setup such an infrastructure. |
|
I can say we used For example we used to connect to RabbitMQ and send the messages after calling Also keep in mind that if you are using Gearman or RabbitMQ or whatever tool you use to defer code execution… you are not deferring the network connection handling, sending data over the wire and what not. I know this is obvious but is often overlooked. So it would be nice to have an standard way of doing this. |
|
This could have been useful recently while implementing a "Poor mans cronjob" system. The solution was to do a custom Response object and do the stuff after send have been called with a Connection: Close header and ignore_user_abort(); (Yes very ugly) |
Commits ------- 7c2f11f Merge pull request #1 from pminnieur/post_response 9f4391f [HttpKernel] fixed DocBlocks 2a61714 [HttpKernel] added PostResponseEvent dispatching to HttpKernel 915f440 [HttpKernel] removed BC breaks, introduced new TerminableInterface 7efe4bc [HttpKernel] Add Kernel::terminate() and HttpKernel::terminate() for post-response logic Discussion ---------- [HttpKernel] Add Kernel::terminate() and HttpKernel::terminate() for post-response logic This came out of a discussion on IRC about doing stuff post-response, and the fact that right now there is no best practice, and it basically requires adding code after the `->send()` call. It's an attempt at fixing it in an official way. Of course terminate() would need to be called explicitly, and added to the front controllers, but then it offers a standard way for everyone to listen on that event and do things without slowing down the user response. --------------------------------------------------------------------------- by stof at 2011/12/06 02:41:26 -0800 We discussed it on IRC and I suggested a way to avoid the BC break of the interface: adding a new interface (``TerminableInterface`` or whatever better name you find) containing this method. HttpKernel, Kernel and HttpCache can then implement it without breaking the existing apps using the component (Kernel and HttpCache would need an instanceof check to see if the inner kernel implements the method) For Symfony2 users it will mean they have to change their front controller to benefit from the new event of course, but this is easy to do. Btw, Silex can then be able to use it without *any* change for the end users as it can be done inside ``Application::run()`` --------------------------------------------------------------------------- by pminnieur at 2011/12/06 11:47:03 -0800 @Seldaek: I opened a pull request so that the discussion on IRC is fulfilled and no BC breaks exist: https://github.com/Seldaek/symfony/pull/1/files --------------------------------------------------------------------------- by fabpot at 2011/12/07 07:59:49 -0800 Any real-world use case for this? --------------------------------------------------------------------------- by Seldaek at 2011/12/07 08:10:31 -0800 Doing slow stuff after the user got his response back without having to implement a message queue. I believe @pminnieur wanted to use it to send logs to loggly? --------------------------------------------------------------------------- by pminnieur at 2011/12/07 09:08:41 -0800 Its a good practice to defer code execution without the introduction of a new software layer (like gearman, amqp, whatever tools people use to defer code execution) which may be way too much just for the goal of having fast responses, whatever my code does. My real world use case which made me miss this feature the first time: > I have a calendar with a scheduled Event. For a given period of time, several Event entities will be created, coupled to the scheduled event (the schedule Event just keeps track of `startDate`, `endDate` and the `dateInterval`). Let's say we want this scheduled Event to be on every Monday-Friday, on a weekly basis, for the next 10 years. This means I have to create `10*52*5` Event entities before I could even think about sending a simple redirect response. If I could defer code execution, I'd only save the scheduled Event, send the redirect response and after that, I create the `10*52*5` entities. The other use case was loggly, yes. Sending logging data over the wire before the response is send doesn't make sense in my eyes, so it could be deferred after the response is send (this especially sucks if loggly fails and i get a 500 --the frontend/public user is not interested in a working logging facility, he wants his responses). --------------------------------------------------------------------------- by mvrhov at 2011/12/07 10:07:03 -0800 This would help significantly, but the real problem, that your process is busy and unavailable for the next request, is still there. --------------------------------------------------------------------------- by fabpot at 2011/12/07 10:15:18 -0800 I think this is the wrong solution for a real problem. Saying "Its a good practice to defer code execution without the introduction of a new software layer" is just wrong. It is definitely a good practice to defer code execution, but you should use the right tool for the job. I'm -1. --------------------------------------------------------------------------- by pminnieur at 2011/12/07 10:25:44 -0800 It should just give a possibility to put unimportant but heavy lifting code behind the send request with ease. With little effort people could benefit from the usage of `fastcgi_finish_request` without introducing new software, using `register_shutdown_function` or using `__destruct `(which works for simple things, but may act weird with dependencies). It should not simulate node.js ;-) I agree that the real problem is not solved, but small problems could be solved easily. I personally don't want to setup RabbitMQ or whatever, maintain my crontab or any other software that may allow me to defer code execution. --------------------------------------------------------------------------- by Seldaek at 2011/12/08 01:08:32 -0800 @fabpot: one could say that on shared hostings it is still useful because they generally don't give you gearman or \*MQs. Anyway I think it'd be nice to really complete the HttpKernel event cycle. --------------------------------------------------------------------------- by pminnieur at 2011/12/08 01:48:57 -0800 not only on shared hostings, sometimes teams/projects just don't have the resources or knowledge or time to setup such an infrastructure. --------------------------------------------------------------------------- by videlalvaro at 2011/12/08 01:53:06 -0800 I can say we used `fastcgi_finish_request` quite a lot at poppen with symfony 1.x. It certainly helped us to send data to Graphite, save XHProf runs, send data to RabbitMQ, and so on. For example we used to connect to RabbitMQ and send the messages _after_ calling `fastcgi_finish_request` so the user never had to wait for stuff like that. Also keep in mind that if you are using Gearman or RabbitMQ or whatever tool you use to defer code execution… you are not deferring the network connection handling, sending data over the wire and what not. I know this is obvious but is often overlooked. So it would be nice to have an standard way of doing this. --------------------------------------------------------------------------- by henrikbjorn at 2011/12/13 01:42:23 -0800 This could have been useful recently while implementing a "Poor mans cronjob" system. The solution was to do a custom Response object and do the stuff after send have been called with a Connection: Close header and ignore_user_abort(); (Yes very ugly)
|
@fabpot the test client should probably be updated too to call terminate when the kernel implements the interface |
This came out of a discussion on IRC about doing stuff post-response, and the fact that right now there is no best practice, and it basically requires adding code after the
->send()call.It's an attempt at fixing it in an official way. Of course terminate() would need to be called explicitly, and added to the front controllers, but then it offers a standard way for everyone to listen on that event and do things without slowing down the user response.