archtechx / tenancy

Automatic multi-tenancy for Laravel. No code changes needed.
https://tenancyforlaravel.com
MIT License
3.65k stars 429 forks source link

tenants:run with jobs combined with queue:work keeps multiple database connections open #1022

Closed jacksnchz closed 1 year ago

jacksnchz commented 1 year ago

Bug description

I am facing a problem while running some jobs with queues combined with tenants:run.

I am executing a company command with a heavy job inside (usually i had a supervisor making this task but has been disabled for debugging), all jobs are sent to Redis before will be processed.

The problemas arrives when command is executed for all tenants, in that case, every time I try to run the command, a new connection is open for each tenant keeping previous one open, and finally provoking a collapse in MySQL throwing "Too many connections" when running via supervisor.

Currently problem is solved using queue:listen instead but the performance is better with queue:work command.

Example: 3 tenants in my system, 2 tenants:run iterations executed and you can see duplicates connections.

capture

Steps to reproduce

Expected behavior

Keep connections open by tenant and use it instead of create new one.

Laravel version

8.83.18

stancl/tenancy version

3.5.7

stancl commented 1 year ago

Currently problem is solved using queue:listen instead but the performance is better with queue:work command

What's the difference between those two in how the DB connections work?

The problem sounds like the DB connections don't get cleaned up, but $tenant->run() should be properly cleaning up the old connection.

jacksnchz commented 1 year ago

Currently problem is solved using queue:listen instead but the performance is better with queue:work command

What's the difference between those two in how the DB connections work?

The problem sounds like the DB connections don't get cleaned up, but $tenant->run() should be properly cleaning up the old connection.

Hi,

Main difference between queue:listen and queue:work is that first one the framework is booted each job (closing connection also) while queue:work framework is booted only one time, so it is much more efficient.

I believe that connection need to be closed by tenant run or manually but I am stuck.

stancl commented 1 year ago
> isset(app('db')->getConnections()['tenant']);
= false

> $tenant->run(fn () => app('db')->getConnections()['tenant']);
= Illuminate\Database\MySqlConnection {#3828} // tenant connection exists inside the run() Closure

> isset(app('db')->getConnections()['tenant']);
= false // cleaned up afterwards

Could it be that you're using tenants:run instead of $tenant->run()? Could you show the exact code?

It might be that in the example above, the connection is removed from Laravel's DatabaseManager but is actually still kept open on MySQL.

jacksnchz commented 1 year ago

Hi, I am using tenants:run and database connection is open in MySQL (listing from SHOW FULL PROCESSLIST)

Here you have an example:

Command class:

class MyCommand extends Command
{
    /**
     * {@inheritdoc}
     * @var string
     */
    protected $signature = 'myCompany:my-command';

    /**
     * Execute the console command.
     *
     * @return void
     */
    public function handle(): void
    {
        MyJob::dispatch();
    }
}

Job class:

class MyJob implements ShouldQueue
{
    use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;

    public function handle()
    {
        logger()->info("running");
    }
}

Steps to reproduce:

  1. php artisan queue:work --tries=3 --sleep=3 --daemon
  2. tenants:run myCompany:my-command
  3. Every time I run tenants:run, a new connection (per tenant) is open keeping previous one open forever
stancl commented 1 year ago

What if you did:

Tenant::all()->runForEach(function () {
    MyJob::dispatch();
});

and called myCompany:my-command without tenants:run.

Does that also reproduce the issue?

jacksnchz commented 1 year ago

Hi again,

Yes, following your advice produce same result

stancl commented 1 year ago

Then my guess is that it's this:

It might be that in the example above, the connection is removed from Laravel's DatabaseManager but is actually still kept open on MySQL.

We'll look into it 👍🏻 @lukinovec could you try to reproduce this in a testing app?

stancl commented 1 year ago

FWIW regarding the solution, I'm not sure if this will be feasible:

Keep connections open by tenant and use it instead of create new one.

We may have to close the connections completely and then reopen them as needed.

jacksnchz commented 1 year ago

Yes maybe keep connections open is not feasible, close connects will be a good choise.

chinmaypurav commented 1 year ago

@jacksnchz Can you try usingTenant Aware Commands?

I am not entirely sure it will work, but I was facing similar problem on my staging (PHP 8).

And this helped. further testing pending

jacksnchz commented 1 year ago

Hi @chinmaypurav,

Thank you for your help but the behaviour using TenantAwareCommand is worst than original idea, more connections are kept open.

My temporary solution was replace queue:work by queue:listen command (loosing performance but it does not overloads the database connections).

stancl commented 1 year ago

@jacksnchz This PR should fix the issue https://github.com/archtechx/tenancy/pull/1128

Could you try testing the change locally to see if it resolves the issue for you?

Essentially just open vendor/stancl/tenancy/src/Bootstrappers/QueueTenancyBootstrapper.php and change line ~73 like this:

- if ($runningTests) {
+ if (true) {

to test the changes from the PR in a simpler way.