feat(core): support frankenphp worker mode#1996
feat(core): support frankenphp worker mode#1996aazsamir wants to merge 12 commits intotempestphp:3.xfrom
Conversation
|
Nice! Thanks for taking this on. |
public/index.php
Outdated
|
|
||
| require_once __DIR__ . '/../vendor/autoload.php'; | ||
|
|
||
| if (function_exists('frankenphp_handle_request')) { |
|
|
||
| require_once __DIR__ . '/../vendor/autoload.php'; | ||
|
|
||
| if (function_exists('frankenphp_handle_request')) { |
There was a problem hiding this comment.
yes, I fully agree. I guess in the final form of this PR I will just remove it, because as far as I understand from reading discord discussion, preparing tempest dockerfiles is another beast to deal with, and probably shouldn't even be handled in this PR. I made that draft-PR to mark/communicate that I'm willing to try work on worker mode support.
There was a problem hiding this comment.
Sounds great, we're looking forward to your contribution! In this case, this comment might also be helpful, this is one of the known limitations.
|
Okay, I'm building a benchmark test suite with different servers
Tests:
Results: benchmark code is available here https://github.com/aazsamir/tempest-worker-test All the tests are stateless, as it seems for me that there is some buggy behaviour about sessions. The more sessions there is, the slower framework is (I think that discovery mechanism tries to scan all the session files). Due to that, I had to change priority of a few middlewares, otherwise they weren't skipped (they were ran before route ignoring them was matched). I am still not marking PR as ready for review, however if someone wants to tell me that the way I'm doing it is stupid, I'm open to hear it! |
aazsamir
requested review from
aidan-casey,
brendt and
innocenzi
as code owners
|
Sorry for leaving this hanging so long, I'm diving into this PR this week! |
Agree that it should be another PR — if needed
What's the reason for having to change this order?
For now it's best to keep them separate IMO, it's indeed easier to finetune worker mode support
Agree |
|
Reading through the PR, I want to propose two alternative approaches.
#[ResetWith(CookieManagerResetter::class)]
final class CookieManager
{
}
final class CookieManagerResetter implements Resetter
{
public function reset(Container $container): void
{
CookieManager::$staticStuff = [];
$container->get(CookieManager::class)->stuff = [];
}
}The benefits of this approach:
Writing out option 1, I wondered: why do we need coupling between resetters and their resettable dependencies? With discovery we can build a list of all classes implementing final class CookieManagerResetter implements Resetter
{
public function reset(Container $container): void
{
CookieManager::$staticStuff = [];
$container->get(CookieManager::class)->stuff = [];
}
}
final class ModelInspectorResetter implements Resetter
{
public function reset(Container $container): void
{
ModelInspector::$inspectors = [];
$container->get(ModelInspector::class)->resetMemoization();
}
}
// …Am I missing something with option 2? |
| final readonly class GenericResetHandler implements ResetHandler | ||
| { | ||
| public function __construct( | ||
| private ResetableContainer $resetableContainer, |
There was a problem hiding this comment.
I find ResetableContainer a bit of a confusing name, because I assumed it was a new container implementation, which it is not. Maybe something like ResetableConfig to keep in line with the rest of the framework would work better?
| Filesystem\write_file( | ||
| filename: $this->getPath($session->id), | ||
| content: serialize($session), | ||
| content: serialize($session->serialize()), |
| $this->data = []; | ||
| } | ||
|
|
||
| public function serialize(): array |
There was a problem hiding this comment.
I prefer this method to be called toArray instead. serialize means the result should be a string, which this is not. This is making an array representation which can be used for serialization
| { | ||
| $sessionIdResolverReflector = new ReflectionClass(CookieSessionIdResolver::class); | ||
| // sessionIdResolver depends on Request, which is not available at the time of initialization, so we create a lazy proxy for it | ||
| // TODO: maybe it should take request as a parameter, not a dependency |
There was a problem hiding this comment.
Usually that's indeed a good reason to move this dependency to the call-site
| use Tempest\Core\Resetable; | ||
|
|
||
| #[Singleton] | ||
| final class RequestHolder implements Resetable |
There was a problem hiding this comment.
What's the reason why we can't reset request and session directly and need these opaque objects in between?
| private readonly Container $container, | ||
| ) {} | ||
|
|
||
| public function reset(): void |
There was a problem hiding this comment.
We could probably move the resetMemoization stuff to HasMemoization
Could be, we can look into that |
4 participants
Initial support for frankenphp worker mode.
I wanted it to be as opaque for framework users as possible, plug it in and everything (well, as most as possible) should just work. Main glue code is done by
OpaqueRequestandOpaqueSession- their state will be distinct for every request, even ifRequestorSessionwere injected as class dependencies.Changes
ResetableandResetableStaticinterfaces, for clearing state after requestSessionasGenericSession, addSessioninterfaceOpaqueRequest/OpaqueSession- request/session classes that may be injected as class dependencies, but their content will be different for every request in worker mode due to their internal state manglingWorkerApplication- http application withfrankenphp_handle_requestsupportWorkerRouter- almost the same asGenericRouter, however it setsOpaqueRequestin container, and matches route before any middleware is ran.Maybe it can be merged withGenericRouter, not sure for nowOpaqueRequestcannot be unserialized, I had to do a few changes in session serializationChange middleware priorities to prevent session creation unless necessaryHow it works
WorkerApplication, boot framework and wait in loop for request. During boot, we registerOpaqueSessionas aSessionlazy proxy - framework is highly coupled withSessionbeing present at any time.$_GETand callsfrankenphp_handle_requesthandle on requestRequestinstance, and pass it toWorkerRouterWorkerRoutersetsRequestintoRequestHoldersingleton and setsOpaqueRequestas aRequestin containerWorkerRoutermatches route and all the middleware stack beginsRequest,OpaqueRequestwill be injected. It is a proxy that will get current request fromRequestHolderunderneath. A bit likeRequestStackin symfony.Session,OpaqueSessionwill be injected. Session will be resolved at call time. This process also depends onRequest.ResetHandleris called and every service implementingResetablewill reset its internal state. Along withRequestHolderunsetting its request it holds.Limitations
This approach seems to work, however it wouldn't work with async servers like Swoole. For that, request/session and related shouldn't be used as class dependencies. Probably should be passed with something like
$requestContext, but it needs a lot of work and breaking changes.Future work
WorkerRoutermatches route at the beginning of request lifecycle, it breaks a few assumptions about it being matched later. I don't think it is a must-have to resolve it now, and this can be done in new PR.WorkerRouterandGenericRouter, especially that they are almost the same. However, it's a bit simpler that way and maybe it will be easier to "stabilize" worker mode if they will be kept separate.DeferredTasksandKernelEvent::SHUTDOWN- they are currently being ran on worker exit, it probably needs a bit more thought put into how it should behave, I don't think it is a must-have for first take.Testing
For testing/debugging, I'm using such dockerfile
then
docker build -t tempest-worker .andAlso, built such repository for testing performance https://github.com/aazsamir/tempest-worker-test/
I will write some unit tests after first round of code review if that's okay, I suspect that still a lot of things can change.