cakephp / queue

A queue-interop compatible Queueing library
MIT License
37 stars 19 forks source link

add DI container support #109

Closed LordSimal closed 1 year ago

LordSimal commented 1 year ago

This PR adds DI container support to tasks.

To do this you have to add the

use \Cake\Queue\Queue\ServicesTrait;

to your task.

With that you have access to the

$service = $this->getService(TestService::class);

method from which you can retrieve objects present inside the DI container.

Its basically the same structure/behavior as its currently available in https://github.com/dereuromark/cakephp-queue/pull/327

othercorey commented 1 year ago

Wouldn't DI support inject the services into the task through the constructor?

LordSimal commented 1 year ago

I'd have to look into that if we want to build it that way.

ndm2 commented 1 year ago

I haven't used the Queue plugin yet, but generally, if DI support is being expanded, I'd like to see CakePHP favor injection wherever possible.

While having proper service location is nice, my worry would be that going down that route could make a switch/upgrade to injection harder in the future.

LordSimal commented 1 year ago

I only used dereuromarks queue plugin till now and I am happy with how the DI services are being accessible via the trait method mentioned above.

But for the core plugins I can definitely see enforcing constructor injected services as a "rule".

LordSimal commented 1 year ago

This now changed to how we are handling service injection into commands. So services are injectable via the jobs constructor and the definition needs to be present inside the services() method of the app/plugin to

But I am not really sure if this can lead to weird results if the worker is running for a longer time of period and the same type of job is being pushed to the queue over and over...

ndm2 commented 1 year ago

But I am not really sure if this can lead to weird results if the worker is running for a longer time of period and the same type of job is being pushed to the queue over and over...

Do you have some specific problem in mind?

markstory commented 1 year ago

But I am not really sure if this can lead to weird results if the worker is running for a longer time of period and the same type of job is being pushed to the queue over and over...

Do you have some specific problem in mind?

One scenario would be services that maintain state and rely on the implicit end of request to 'cleanup' the possibility of shared state.

My position on this is that we should document that services used in workers are long lived and shouldn't retain state. At the end of the day application developers are responsible for using services in queue workers safely.

In the future if this becomes a pain point we can address it then and find a way to reset the container instances after each job is processed.

LordSimal commented 1 year ago

One scenario would be services that maintain state and rely on the implicit end of request to 'cleanup' the possibility of shared state.

Thinking once more about it: Doesn't the DI Container create new instances by default whenever you ask for a specific service? Only if someone defines it as a shared service inside the services() method it will then be a singleton and therefore have state for each job/message process run.

But other shared state could definitely mess up how users expect it work.

markstory commented 1 year ago

Only if someone defines it as a shared service inside the services() method it will then be a singleton and therefore have state for each job/message process run.

Yes, someone will do it eventually :smile: