Make sure sessions are cleaned after processing a request.

[mathieudz]

Sessions must also be cleaned up when Symfony throws an exception to avoid session leaking.

[mathieudz]

Should fix #130

[andig]

Could you add a test that shows the problem and then confirms the fix? @dnna has just added some test cases to the httpkernel bridge for Symfony in case the test needs to go there?

[dnna]

To be honest I have never really liked this catch block. Its not just the session, basically none of the postHandle functions of the bootstrap are executed.

Maybe it would be better to kill the worker and just log something in this scenario? Normally the the kernel shouldn't be throwing unhandled exceptions anyway.

[mathieudz]

It seems that the kernel throws an exception when a class is not found. Not very unusual in a scripting language.

[mathieudz]

It makes much sense to kill the worker. Now that I think of it, many issues I have encountered are related. E.g. a database transaction that is not committed or rolled back due to such an exception: my workaround is to send a rollback statement for every new request. Until now I had no idea what caused it. Killing the worker would solve that much better.

[dnna]

@mathieudz yes exactly. My point was that if the kernel throws an exception, it means something went wrong and we don't really know if the kernel is suitable to be reused in the next request (its for the same reason that doctrine closes the entity manager if an SQL error occurs). So killing the worker is probably the best solution for this issue and should improve stability in general.

[andig]

Should we also revisit the doctrine handling currently implemented then?

[dnna]

I think the current Doctrine handling is fine, it addresses issues that can occur even if the kernel does not throw an exception (for example Symfony may catch the exception and show its own error page).

[mathieudz]

I still have session leaking with this patch. Anyone an idea how a worker can be killed from HttpKernel?

[dnna]

Good question, the bridge does not have access to the slave.
I think we should remove the catch block from the bridge and allow the exception to go up the stack to https://github.com/php-pm/php-pm/blob/master/src/ProcessSlave.php#L428. We could then add a catch block there that logs it and calls $this->shutdown().

@andig what do you think?

[mathieudz]

The reason that this patch doesn't work is because it only closes the session for PHP, but Symfony still considers it open and still has all data loaded.

[andig]

The reason that this patch doesn't work is because it only closes the session for PHP, but Symfony still considers it open and still has all data loaded.

This would no longer be a topics if we kill the worker? Out of plain security considerations I like he suggestion by @dnna. Let the handle exception bubble and close the worker.

Otherwise we would need to do something in postHandle or preHandle if https://github.com/php-pm/php-pm-httpkernel/blob/master/Bridges/HttpKernel.php#L133 isn't sufficient for Symfony.

[dnna]

@mathieudz lets make this PR just remove the catch block completely, so the exception is allowed to bubble up. I will make a separate PR in https://github.com/php-pm/php-pm to catch it and shutdown the worker.

[andig]

The reason that this patch doesn't work is because it only closes the session for PHP, but Symfony still considers it open and still has all data loaded.

@mathieudz Looking at what you wrote again I'm wondering- apart from proper exception handling- if we're missing anything for properly handling symfony sessions?

[mathieudz]

@andig IMHO the real problem here is

1. Symfony allows terminating a request improperly (not calling all listeners) - maybe it's not possible?
   1. Leaving database connections open
   2. Leaving sessions open
   3. ...
2. The Session service is stateful service and needs to switch state for each request. In my own code I avoid stateful services and inject the session data & request data everywhere it's needed.

A solution for the latter would be to reset all stateful Symfony services (request_stack, session, ...), but the former can only be solved by php-pm by killing the worker. Symfony does rely on the share-nothing architecture when things go wrong.

[dnna]

I think we should limit the scope of this PR to only handle case 1 (request terminates improperly and not all listeners are executed) by killing the worker as discussed. Basically we remove the try-catch block from HttpKernel and let the exception bubble up to ProcessSlave (and catch it with the other PR).

We can open a separate issue to discuss case 2, though personally I think its mostly fine, otherwise we would have noticed a lot more issues if session or request leaked under the normal workflow.

[mathieudz]

OK for solving case 1 - that's my idea too. The PR has been updated for it.

[andig]

Reminder: need to require min ppm version when releasing this.

[mathieudz]

@andig Reminding you of the reminder :)

[andig]

Darn. Why did we want to do this? It would typically pick up latest anyway?

[mathieudz]

I think that latest phppm release + latest httpkernel release will crash on unhandled Symfony exceptions. This PR lets the exceptions bubble up, so you also need the PR of phppm that catches it.

[andig]

Oh yeah... fixed via 34327ed