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

Container::when does not work for handling Jobs. #23602

Closed
frankdejonge opened this issue Mar 18, 2018 · 9 comments
Closed

Container::when does not work for handling Jobs. #23602

frankdejonge opened this issue Mar 18, 2018 · 9 comments
Assignees
Labels

Comments

@frankdejonge
Copy link
Contributor

  • Laravel Version: 5.6.12
  • PHP Version: 7.2.1
  • Database Driver & Version: Not relevant

Description:

I want to contextually bind a service to the handle method of a Job. When

Steps To Reproduce:

  1. Have a Job that takes a service defined as an interface.
  2. Bind a service contextually to the job.
$this->app->when(Job::class)->needs(Contract::class)->give(function () { return Concrete(); });

Result

The queue worker endlessly tries to process the message, failing and then retrying. It was not easy to determine this was the problem either. When wiring cannot be resovled I don't expect it to be handled the same way "failed job processing" is handled.

screen shot 2018-03-18 at 15 30 11

@devcircus
Copy link
Contributor

I may be wrong, but I believe the contextual binding only applies to constructor injection.

I know it isn't exactly what you're looking for, but check out this PR. It may help.

@frankdejonge
Copy link
Contributor Author

@devcircus that does the trick. There's no documentation for that feature it seems and the IDE helpers didn't hint that method, so I had no clue. But, besides that I think think the error in resolving the job should not result in a retry loop.

@driesvints
Copy link
Member

@devcircus I'll try to send in a PR to the docs for this because this hasn't been documented it seems.

@frankdejonge you're right. I'll update the docs to clarify this in the contextual binding section and link to the new docs which I'm going to send in for the referenced PR.

You're also right that the retry perhaps isn't wanted for a container resolving issue. Do you perhaps know which exception is thrown? We might be able to catch it and properly handle it.

@driesvints driesvints added the bug label Nov 20, 2018
@driesvints
Copy link
Member

@driesvints
Copy link
Member

driesvints commented Dec 5, 2018

@devcircus @frankdejonge it's now documented at least: laravel/docs@d17e701

Will further investigate this issue as soon as I get to it.

@wonderlandzor
Copy link
Contributor

The PR seeks that references this bug seeks to resolve this issue. The specific case for jobs is something that I run into regularly.

@devcircus seems to disagree with my approach, please share your thoughts in the PR if you can.

@driesvints
Copy link
Member

@nmokkenstorm the actual issue here is that the worker endlessly retries the job for a container resolver failure.

@wonderlandzor
Copy link
Contributor

wonderlandzor commented Nov 11, 2019

@driesvints I'm aware that that's why the issue was raised. The cause of the specific failure however, is a problem I've encountered before, and am trying to solve in the PR. Binding a closure feels like a workaround to me.

@driesvints driesvints changed the title [5.6] Container::when does not work for handling Jobs. Container::when does not work for handling Jobs. Mar 17, 2020
@taylorotwell
Copy link
Member

There is not really an issue. The retry loop isn't endless when tries is set:

image

There are situations when retrying would indeed be warranted for example if the constructor of the injected service connects to an external resource like Redis and fails to resolve because Redis is down. I'm not saying that is great design but in that case retry would be warranted.

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

6 participants
@taylorotwell @frankdejonge @driesvints @devcircus @wonderlandzor and others