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

Cannot use finally method of Laravel Batch Job inside an Activity #91

Closed
phuclh opened this issue May 25, 2023 · 7 comments · Fixed by #97
Closed

Cannot use finally method of Laravel Batch Job inside an Activity #91

phuclh opened this issue May 25, 2023 · 7 comments · Fixed by #97

Comments

@phuclh
Copy link

phuclh commented May 25, 2023

I have a batch job that is dispatched in an Activity. When the batch job is finished, I would like to trigger the Workflow to fire the next Activity. However, I got an error when using finally method.

CleanShot 2023-05-25 at 16 47 42@2x

Here is my Activity code:

Bus::batch($jobs)
->allowFailures()
->finally(function () {
    $this->storedWorkflow->toWorkflow()->setReady(true);
})
->dispatch();

Here is my Workflow code:

yield ActivityStub::make(ActivityOne::class);

yield WorkflowStub::await(fn () => $this->ready);

yield ActivityStub::make(ActivityTwo::class);

Updated: Seems this is an error of Laravel, not this package. If I use $this inside the finally method, it will throw that error. So here is the walkaround:

$id = $this->workflowId();

Bus::batch($jobs)
->allowFailures()
->finally(function () use ($id) {
    $workflow = StoredWorkflow::find($id);
    WorkflowStub::fromStoredWorkflow($workflow)->setReady(true);
})
->dispatch();

However, there is something wrong with serializing the options column in job_batches table. When I run Bus::findBatch($batchID), it throw this error.

CleanShot 2023-05-25 at 22 57 09@2x

Probably, the Larvel serializing was conflicted with the Workflow serializing.

@tuktukvlad
Copy link

Hi
Since batch callbacks are serialized and executed at a later time by the Laravel queue, you should not use the $this variable within the callbacks:
->finally(function () { $this->storedWorkflow->toWorkflow()->setReady(true); })

@phuclh
Copy link
Author

phuclh commented May 26, 2023

Hi Since batch callbacks are serialized and executed at a later time by the Laravel queue, you should not use the $this variable within the callbacks: ->finally(function () { $this->storedWorkflow->toWorkflow()->setReady(true); })

Yes, I already used WorkflowStub::fromStoredWorkflow() to fix that.

However, do you know why the serialization is broken when using Batch with Workflow? Right now, I have to override the serialize, and unserialize method in DatabaseBatchRepository to make it works.

protected function serialize($value): string
{
    return Y::serialize($value);
}

protected function unserialize($serialized): mixed
{
    return Y::unserialize($serialized);
}

@rmcdaniel
Copy link
Contributor

It sounds like you found a workaround. Is this still a blocker for you?

@rmcdaniel
Copy link
Contributor

@phuclh I have figured out the issue. A fix is coming soon. However you can do this part...

->finally(function () use ($id) {
    $workflow = StoredWorkflow::find($id);
    WorkflowStub::fromStoredWorkflow($workflow)->setReady(true);
})

like this instead...

->finally(function () use ($id) {
    WorkflowStub::load($id)->setReady(true);
})

And it will be simpler for you.

@phuclh
Copy link
Author

phuclh commented Jun 1, 2023

Nice, thank you for the simpler solution. Have you had a chance to look at the issue when serialization is broken when using Batch with Workflow? Right now, I have to override the serialize, and unserialize methods in DatabaseBatchRepository class to make it works.

protected function serialize($value): string
{
    return Y::serialize($value);
}

protected function unserialize($serialized): mixed
{
    return Y::unserialize($serialized);
}

@rmcdaniel
Copy link
Contributor

@phuclh Yes sir, I figured it out. I have a PR coming to fix it in the next release. 🎉

@phuclh
Copy link
Author

phuclh commented Jun 1, 2023

Great. Thank you so much 👍😊

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 a pull request may close this issue.

3 participants