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

[8.x] Introduce scoped instances #37521

Merged
merged 4 commits into from
Jun 8, 2021
Merged

[8.x] Introduce scoped instances #37521

merged 4 commits into from
Jun 8, 2021

Conversation

themsaid
Copy link
Member

@themsaid themsaid commented May 28, 2021

Scoped instances are singletons that must be flushed while switching the application context. e.g.

$this->app->scoped('service', function (){
    return new Service(...);
});

1- On a new Octane request.
2- Before processing a new queued job.

In this PR, I only reset the scope before processing queued jobs. I original reset the scope inside HttpKernel::handle() but I figured the scope needs resetting earlier than that when handling an HTTP request. So if this request was merged, I'll add the scope resetting to Octane once the sandbox instance is cloned.

@deleugpn
Copy link
Contributor

If there's any chance of having the reset of Http instances within the framework itself it would be awesome because we get to use this outside of Octane e.g. within a long-lived Http Kernel in AWS Lambda with Bref.

I already have a use case that would benefit from the worker reset

@themsaid
Copy link
Member Author

@deleugpn you can call $app->resetScope() in bref. Thing is in Octane, resetting the scope needs to happen before calling $kernel->handle(). I assume it'll be the same in bref as well.

@taylorotwell
Copy link
Member

So is the main goal for this PR to allow packages to define bindings that should be flushed before handling another "task"?

@themsaid
Copy link
Member Author

themsaid commented Jun 2, 2021

@taylorotwell yes. And also allow app developers to choose between singleton and scoped while binding to the container. That way they won't forget to flush the service if it should be flushed between tasks.

@taylorotwell
Copy link
Member

taylorotwell commented Jun 7, 2021

Instead of injecting a closure into the queue system, I wonder if the FoundationServiceProvider or QueueServiceProvider could just register a listener for the existing Looping event to flush the scoped instances? Then the queue component doesn't need to be aware of this change at all.

@themsaid
Copy link
Member Author

themsaid commented Jun 8, 2021

@taylorotwell I noticed we don't use listeners in core ServiceProviders. I think it's because tracking those listeners won't be easy when you're browsing the code and try to debug stuff.

If it's ok to use a listener in a service provider then yes I think we can do that in the QueueServiceProvider.

@Brenneisen
Copy link
Contributor

@themsaid is there any way to refresh the scoped instances after flushing in other services where they are injected as dependency?

@themsaid
Copy link
Member Author

@Brenneisen I'm afraid not. You'll need to make those other services scoped as well.

@hubertnnn
Copy link

@themsaid
Looking at the code only one scope can be active at a time.
This is a good first step, since package developers will start using the scope function instead of singleton.
But what if you need to switch back and forth between 2 scopes (eg. in swoole co-processing)?

@themsaid
Copy link
Member Author

themsaid commented Jul 1, 2021

@hubertnnn coroutine support requires a lot of work and changes. When it's time we tackle it we'll have multiple scopes.

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

Successfully merging this pull request may close these issues.

5 participants