Skip to content
This repository

[HttpKernel] Add Kernel::terminate() and HttpKernel::terminate() for post-response logic #2791

Merged
merged 1 commit into from over 2 years ago

7 participants

Jordi Boggiano Christophe Coevoet Pierre Minnieur Fabien Potencier Miha Vrhovnik Alvaro Videla Henrik Bjørnskov
Jordi Boggiano
Collaborator

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.

src/Symfony/Component/HttpKernel/HttpKernelInterface.php
... ...
@@ -44,4 +44,11 @@
44 44
      * @api
45 45
      */
46 46
     function handle(Request $request, $type = self::MASTER_REQUEST, $catch = true);
  47
+
  48
+    /**
  49
+     * Terminates a request/response cycle
  50
+     *
  51
+     * Should be called before shutdown, but after sending the response
  52
+     */
  53
+    function terminate();
10
Christophe Coevoet Collaborator
stof added a note December 06, 2011

HttpKernelInterface was marked as stable so this is a BC break (simple way to see it: Silex needs to be updated after this change)

Jordi Boggiano Collaborator
Seldaek added a note December 06, 2011

I realize that, but I think we need to decide if this is needed first, and then how to deal with BC later.

Christophe Coevoet Collaborator
stof added a note December 06, 2011

I think we should try to keep the HttpKernelInterface as simple as possible

Jordi Boggiano Collaborator
Seldaek added a note December 06, 2011

We could dispatch directly from the Kernel to avoid adding it to the interface, but IMO it belongs in the HttpKernel.

Christophe Coevoet Collaborator
stof added a note December 06, 2011

it cannot be dispatch from the Kernel directly as it would stay before $response->send() if it is done inside handle :)

that's the exact problem i stumbled upon ... ;-)

Jordi Boggiano Collaborator
Seldaek added a note December 06, 2011

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(...);
}
Christophe Coevoet Collaborator
stof added a note December 06, 2011

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).

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.

Christophe Coevoet Collaborator
stof added a note December 06, 2011

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/HttpKernel/HttpKernelInterface.php
... ...
@@ -44,4 +44,11 @@
44 44
      * @api
45 45
      */
46 46
     function handle(Request $request, $type = self::MASTER_REQUEST, $catch = true);
  47
+
  48
+    /**
  49
+     * Terminates a request/response cycle
  50
+     *
  51
+     * Should be called before shutdown, but after sending the response
2
Christophe Coevoet Collaborator
stof added a note December 06, 2011

HttpKernelInterface does not have any shutdown

Jordi Boggiano Collaborator
Seldaek added a note December 06, 2011

Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php
... ...
@@ -216,6 +216,18 @@ public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQ
216 216
     }
217 217
 
218 218
     /**
  219
+     * Terminates a request/response cycle
  220
+     *
  221
+     * Should be called before shutdown, but after sending the response
  222
+     *
  223
+     * @api
  224
+     */
  225
+    public function terminate()
  226
+    {
  227
+        $this->kernel->terminate();
1
Christophe Coevoet Collaborator
stof added a note December 06, 2011

issue here: when the cache was used, the inner kernel is not booted, so this method will throw a LogicException

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Christophe Coevoet
Collaborator

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()

Pierre Minnieur

@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

Jordi Boggiano Merge pull request #1 from pminnieur/post_response
[HttpKernel] Add Kernel::terminate() and HttpKernel::terminate() for post-response logic (without the BC break)
7c2f11f
Fabien Potencier
Owner

Any real-world use case for this?

Jordi Boggiano
Collaborator

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?

Pierre Minnieur

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).

Miha Vrhovnik

This would help significantly, but the real problem, that your process is busy and unavailable for the next request, is still there.

Fabien Potencier
Owner

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.

Pierre Minnieur

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.

Jordi Boggiano
Collaborator

@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.

Pierre Minnieur

not only on shared hostings, sometimes teams/projects just don't have the resources or knowledge or time to setup such an infrastructure.

Alvaro Videla

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.

Henrik Bjørnskov

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)

Fabien Potencier fabpot referenced this pull request from a commit December 15, 2011
Fabien Potencier merged branch Seldaek/post_response (PR #2791)
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)
abad85c
Fabien Potencier fabpot merged commit 7c2f11f into from December 15, 2011
Fabien Potencier fabpot closed this December 15, 2011
Christophe Coevoet
Collaborator

@fabpot the test client should probably be updated too to call terminate when the kernel implements the interface

mmucklo mmucklo referenced this pull request from a commit December 15, 2011
Fabien Potencier merged branch Seldaek/post_response (PR #2791)
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)
330408e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Dec 06, 2011
Jordi Boggiano Merge pull request #1 from pminnieur/post_response
[HttpKernel] Add Kernel::terminate() and HttpKernel::terminate() for post-response logic (without the BC break)
7c2f11f
This page is out of date. Refresh to see the latest.

Showing 0 changed files with 0 additions and 0 deletions. Show diff stats Hide diff stats

Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.