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

Bus::chain Skipping jobs with readonly non-scalar properties #51877

Closed
shaffe-fr opened this issue Jun 21, 2024 · 13 comments · Fixed by laravel/serializable-closure#87
Closed

Comments

@shaffe-fr
Copy link
Contributor

shaffe-fr commented Jun 21, 2024

Laravel Version

10.x and 11.x

PHP Version

8.1, 8.2 and 8.3

Database Driver & Version

No response

Description

Description of the Issue

When a Bus::chain contains a job with a readonly promoted non-scalar property, only the first job or batch in the chain is executed. The subsequent jobs or batches appear to be discarded. Additionally, even if the chain is dispatched on a non-default queue, the jobs still run on the default queue.

No exceptions were found in the logs.

Environment

Laravel versions: 10.x and 11.x
PHP versions: 8.1, 8.2, and 8.3

Evidence

Failing test PR : #51878
A repository that reproduces the problem can be found here: shaffe-fr@f1b3cad

Here is a screenshot illustrating the issue:
image

Steps To Reproduce

  1. Create a Bus::chain with at least one job having a readonly promoted non-scalar property.
  2. Dispatch the chain.
  3. Observe that only the first job or batch is executed, and subsequent jobs/batches are not processed.
  4. Note that jobs run on the default queue, regardless of the specified queue.
@shaffe-fr shaffe-fr changed the title Jobs with readonly Class properties discarded in Bus::chain Bus::chain Skipping jobs with readonly non-scalar properties Jun 21, 2024
@driesvints
Copy link
Member

Thanks @shaffe-fr. It's not by consequence that this is related to the enum usages in your PR?

@shaffe-fr
Copy link
Contributor Author

Hi @driesvints, indeed, I had the same issue with classes.
I added another failing test case with a simple class in the PR #51878.

Copy link

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

@rust17
Copy link
Contributor

rust17 commented Jul 30, 2024

I believe this issue is due to the package(laravel/serializable-closure) not supporting the readonly property.

@juse-less
Copy link

juse-less commented Jul 31, 2024

The issue I have with chained batches is that they stop executing whenever I have a job that uses a class type for its properties.
As soon as the next job in a chained batch has a class property, the chain is removed from the queue and marked as completed.

I've tried many combinations of visibility, readonly, promoted properties, non-promoted properties, serialising and unserialising the classes myself
Nothing seems to work, other than removing the properties.

All the jobs/batches in the chain are correctly displayed in the payload in both the Redis and database drivers.

@rust17
Copy link
Contributor

rust17 commented Aug 1, 2024

@juse-less Could you provide some example codes? I can't reproduce it.

@juse-less
Copy link

@rust17 Hmm.
I might have a different issue entirely, but the behaviour sounds like the same as this issue.

I'll try your fix today, just to be sure I'm not missing something.
I'll then create a minimal reproducer.

@shaffe-fr
Copy link
Contributor Author

I tried your fix with the code in the PR #51878 but it does not seems to work.

@rust17
Copy link
Contributor

rust17 commented Aug 1, 2024

@shaffe-fr Hmm. This looks strange. I tried it and it works for me. You can see the results here. Could you consider accepting this PR? Try to trigger the GitHub Action and check if there are the same errors.

@shaffe-fr
Copy link
Contributor Author

I tried a quick and dirty test: I added the if statement in the Native.php file modification in the vendor.

I'll try it directly in my project using your version of laravel/serializable-closure.

@shaffe-fr
Copy link
Contributor Author

I confirm that I don't have the problem in my project with this fix!

@juse-less
Copy link

Same here. Seems like it actually fixes the problem.

I tested on the state I left things in a couple weeks ago, but definitely works now.
I first tested without installing @rust17 branch, and it still failed.
After installing the branch, it now goes through a chain of 10 batches, most of which use class properties- and arguments.

I'm gonna reset all my changes I did while debugging, but I'm confident this does solve it altogether.

@shaffe-fr
Copy link
Contributor Author

Hey @driesvints, looks like we have a winner here :)
I'll close the issue when the PR laravel/serializable-closure#87 will be merged.

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

Successfully merging a pull request may close this issue.

4 participants