Skip to content
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

Controllers are not dropped per request #587

Closed
olivernybroe opened this issue Sep 27, 2022 · 9 comments
Closed

Controllers are not dropped per request #587

olivernybroe opened this issue Sep 27, 2022 · 9 comments
Assignees
Labels

Comments

@olivernybroe
Copy link

olivernybroe commented Sep 27, 2022

  • Octane Version: v1.3.1
  • Laravel Version: v9.31.0
  • PHP Version: 8.1.10
  • Server & Version: Swoole 4.8.7
  • Database Driver & Version:

Description:

A controller will have the same instance throughout the whole lifetime of the application.

This doesn't sounds like a problem when first hearing it, the problem is when you start to do dependency injections via the constructor on a controller.

So we had a controller for handling socialite called SocialiteController, which took Laravel\Socialite\Contracts\Factory in through the constructor, in our redirect method we called $this->socialite->driver('someDriver') and did the redirect.

This would result in a Target class [request] does not exist. on the second request to this controller. Reason for this is that the Container instance on the SocialiteManager (the class that implements the factory) would point to the old app, and not the app for the new request, as the instance of the Manager is saved on the controller itself.

This is not a problem with many other dependencies, as they do not depend on request, however SocialiteManager passed an instance of Request down to each driver.

It looks like A workaround was created two years ago, in the class named PrepareSocialiteForNextOperation, however the following line always returns false, so the code for resetting the container and removing the old drivers never happens.

if (! $event->sandbox->resolved(Factory::class)) {

This issue seems to not be limited to SocialiteManager, but is something we also experienced with AuthManager, resulting in people being logged in on others accounts.

Our solution for unblocking us was to remove all of these dependencies into each controller method instead of having them shared at the constructor. However I feel that this might make sense to change, so all controllers are not stored in the application, but a new instance of the controller is created on every single request.

@taylorotwell
Copy link
Member

taylorotwell commented Sep 29, 2022

This should only be the case if your controller is registered as a singleton? Or you are injecting the container itself somehow?

It is definitely not the case in Octane by default - which you can probably see if you try it on a fresh Laravel application using Breeze or something?

@olivernybroe
Copy link
Author

The controller is not a singleton and I am just injecting the Laravel\Socialite\Contracts\Factory.

However I just tried your idea with a fresh Laravel application, with Octane installed and a simple invokable controller to return the dashboard.

I added a logging into the constructor and the invokable method and this is the result. (the number is the spl object id)
image

Here is the example app I created for showcasing it (changed logger to stderr)
https://github.com/olivernybroe/octane-example

@olivernybroe
Copy link
Author

To show the problem I was talking about with Socialite factory, I have created a branch for replicating that issue.
Here is a screenshot of the object id's which shows that the id of the request object does not change in the github driver.
image

https://github.com/olivernybroe/octane-example/tree/socialite

@taylorotwell
Copy link
Member

taylorotwell commented Sep 29, 2022

I don't think dumping the object ID is a good way to determine the same instance is being used across requests? For instance, this will dump the same object ID on every request even without using Octane even though it is obvious a new instance is being used every request:

CleanShot 2022-09-29 at 14 49 41@2x

@taylorotwell
Copy link
Member

taylorotwell commented Sep 29, 2022

In this example, a new random string is returned on each request when using Octane + Roadrunner locally. That tells me the controller is not being reused across requests:

CleanShot 2022-09-29 at 15 07 48@2x

@olivernybroe
Copy link
Author

@taylorotwell I see your point about object id as it can in theory get the exact same memory location, however in my examples I posted I also did a log in the constructor and that is only happening once.

I also just tried the example you shared here and I get the same value every time.

Kapture.2022-09-30.at.08.40.33.mp4

I did realise one difference and that is you were using RoadRunner, I just retried with RoadRunner and that does not reproduce my issue. So it seems like this is limited to Swoole actually. I just tried upgrading to Swoole 5 also, and the issue is still there.

@driesvints
Copy link
Member

@olivernybroe Nuno and Taylor came up with a fix already which we're probably gonna tag somewhere today:

#589
laravel/framework#44386

@olivernybroe
Copy link
Author

@driesvints Ah cool!

I just tried installing dev laravel and Nuno's branch with the fix for octane and it seems to solve it! 🎉

Thanks team ❤️

@driesvints
Copy link
Member

Fixed now, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants