Skip to content

[9.x] Release lock for job implementing ShouldBeUnique that is dispatched afterResponse() - #46806

Merged
taylorotwell merged 9 commits into
laravel:9.xfrom
cosmastech:fix/after-response-unique-lock
Apr 18, 2023
Merged

[9.x] Release lock for job implementing ShouldBeUnique that is dispatched afterResponse()#46806
taylorotwell merged 9 commits into
laravel:9.xfrom
cosmastech:fix/after-response-unique-lock

Conversation

@cosmastech

@cosmastech cosmastech commented Apr 17, 2023

Copy link
Copy Markdown
Contributor

To resolve #46537

  • Changed Dispatchable@dispatchAfterResponse() so that PendingDispatch@shouldDispatch() checks for lock, rather than just pushing immediately to the Application::$terminatingCallbacks array. Could move this to a separate PR, as maybe you would consider it a breaking change. Seems kind of strange that Job::dispatchAfterResponse() doesn't check for uniqueness, but Job::dispatch()->afterResponse() does.
  • Moves the handling of middleware/unique locks out of CallQueuedHandler@call() to its own method CallQueuedHandler@handle(). I named the method handle() so that it could be more easily used with Bus\Dispatcher@dispatchNow() (which is expecting the $handler argument to implement either __invoke() or handle().
    • I think normally I would move this to its own action class or something, but really wanted to make as few changes as possible to get this working.

Jobs dispatched after response should now also be routed through middleware as well. Might make sense to add a test case for that, just let me know.

@cosmastech
cosmastech marked this pull request as draft April 17, 2023 13:30
@cosmastech cosmastech changed the title [9.x] Release lock for job implementing ShouldBeUnique that is dispatchedAfterResponse [9.x] Release lock for job implementing ShouldBeUnique that is dispatched afterResponse() Apr 17, 2023
@cosmastech
cosmastech marked this pull request as ready for review April 17, 2023 14:32
@taylorotwell

taylorotwell commented Apr 17, 2023

Copy link
Copy Markdown
Member

Is this not as simple as just changing dispatchAfterResponse on the Bus Dispatcher to use dispatchSync instead of dispatchNow?

dispatchNow is not recommended and is quasi deprecated because of the fact that it doesn't apply middleware, etc. That simple change fixes the issue for me.

public function dispatchAfterResponse($command, $handler = null)
{
    $this->container->terminating(function () use ($command, $handler) {
        $this->dispatchSync($command, $handler);
    });
}

@cosmastech

Copy link
Copy Markdown
Contributor Author

Is this not as simple as just changing dispatchAfterResponse on the Bus Dispatcher to use dispatchSync instead of dispatchNow?

dispatchNow is not recommended and is quasi deprecated because of the fact that it doesn't apply middleware, etc. That simple change fixes the issue for me.

image

You're right.

I've reverted the other changes I made, though I kept the change to Dispatchable::dispatchAfterResponse(). Without it, there's never a call to PendingDispatch@shouldDispatch()

Tweaked the test cases slightly to use both Job::dispatch()->afterResponse() and Job::dispatchAfterResponse();

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.

2 participants