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

Job not running as tenant #1277

Closed
ju5t opened this issue Dec 27, 2024 · 10 comments
Closed

Job not running as tenant #1277

ju5t opened this issue Dec 27, 2024 · 10 comments
Assignees

Comments

@ju5t
Copy link

ju5t commented Dec 27, 2024

Bug description

Even though a job is dispatched with:

$tenant->run(fn () => dispatch(new Job));

The tenant() inside the job is set to null.

Steps to reproduce

We have the following code:

new DispatchAsTenant(AssignCustomerProducts::class)

DispatchAsTenant loops over the existing tenants and it is meant to dispatch the AssignCustomerProducts-job for each one of them. The code of DispatchAsTenant is:

class DispatchAsTenant implements ShouldQueue
{
    use Dispatchable;
    use InteractsWithQueue;
    use Queueable;

    /** @param class-string<ShouldQueue> $jobName */
    public function __construct(protected string $jobName)
    {
        //
    }

    public function handle(): void
    {
        Tenant::cursor()->each(function (Tenant $tenant) {
            $tenant->run(fn () => dispatch(new $this->jobName));
        });
    }
}

Inside the handle method of the AssignCustomerProducts-job I have:

dd(tenant());

This returns:

null // app/Jobs/AssignCustomerProducts.php:29

Expected behavior

I expect tenant() to be the tenant that was active when dispatching the job.

Laravel version

v11.29.0

stancl/tenancy version

dev-master, but not the latest version. This app is on PHP 8.2 and the beta release requires PHP 8.4. We're on the version that was available on the 20th of December.

@ju5t ju5t added the bug Something isn't working label Dec 27, 2024
@stancl
Copy link
Member

stancl commented Dec 28, 2024

If you don't use this DispatchAsTenant wrapper and just try to dispatch a job for a tenant, does it run in the tenant context?

@ju5t
Copy link
Author

ju5t commented Dec 28, 2024

@stancl It's also missing the tenant's detail when I don't use the wrapper and dispatch it from a tenant's tinker session.

I just did some more tests with Log::info(tenant()) on the AssignCustomerProducts-job:

  • When called from the __construct it returns the tenant.
  • When called from the handle()-method it returns null.

@stancl
Copy link
Member

stancl commented Dec 28, 2024

Then that means your queue setup is broken overall and this is unrelated to the DispatchAsTenant logic. When solving problems like this it's best to remove unrelated variables from the equation to see more clearly where the problem may be. Let me confirm that queues work fine in v4.

@ju5t
Copy link
Author

ju5t commented Dec 28, 2024

@stancl some background detail of our setup:

  • We're using Redis for our queues and the RedisTenancyBootstrapper is enabled.
  • The REDIS_QUEUE_CONNECTION queue is not in tenancy.redis.prefixed_connections as the docs mentioned.

I do not specify a connection when dispatching the job:

dispatch(new AssignCustomerProducts);

If there are any details that can help find the issue let me know.

@stancl
Copy link
Member

stancl commented Dec 28, 2024

Try getting tenant-aware queued jobs to work in a fresh app, then compare what's different in your setup.

Here's a recording of me setting everything up from scratch, testing various drivers etc. Bit unstructured so feel free to skip through it https://asciinema.org/a/DLjf9W6IGChWQKctesTev7yMc

@ju5t
Copy link
Author

ju5t commented Dec 29, 2024

@stancl there's a small difference in our setups. You're using $tenant->enter() where I use $tenant->run().

This works:

$tenant = Tenant::first();
$tenant->enter();
dispatch(new AssignCustomerProducts);

This doesn't:

$tenant = Tenant::first();
$tenant->run(fn () => dispatch(new AssignCustomerProducts));

When I use enter() and leave() instead of run() inside DispatchAsTenant that works too.

@stancl
Copy link
Member

stancl commented Dec 29, 2024

That observation indicates this is related to how jobs get dispatched in PendingDispatch::__destruct(). I just tested this using the longer syntax closure and funnily it worked. On second thought it makes sense because we are returning the dispatch.

function my_function($tenant) {
    $tenant->run(function () {
        dispatch(new FooJob);
        // PendingDispatch destroyed here
    });
}

function my_function($tenant) {
    // implicit return value
    /* $_ = */ $tenant->run(fn() => dispatch(new FooJob));

    // PendingDispatch destroyed here - once we're back in the central context
}

This should work as well $tenant->run(fn() => dispatch(new FooJob) && 1);.

@stancl
Copy link
Member

stancl commented Dec 29, 2024

I'm inclined to close this since this is just how Laravel and PHP work, but the snippet you were trying to get to work here was written by me. Meaning it's easy even for me to make a mistake here and forget how dispatches work with closure return values.

So I'm thinking to avoid wasting people's time we could take some steps in the package to automatically resolve this behind the scenes. E.g. run(fn () => dispatch(...)) should just return null, the run() method should destruct the return value if it's a PendingDispatch.

@ju5t
Copy link
Author

ju5t commented Dec 29, 2024

I would have never expected this to not work with short closures. Learning new things every day.

So I'm thinking to avoid wasting people's time we could take some steps in the package to automatically resolve this behind the scenes.

That's probably the easiest to prevent questions down the line. I'm sorry if it felt like I'm wasting your time with this.

@stancl
Copy link
Member

stancl commented Dec 29, 2024

No worries, again since it's code I wrote I would've expected it to work too. And I think many people who use the package would too, so to avoid wasting their time going down this rabbit hole I'll probably just add special handling to run() and similar methods.

@stancl stancl added usability v4 and removed bug Something isn't working labels Dec 30, 2024
@stancl stancl closed this as completed Jan 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants