Skip to content

queue:work is not persistent #443

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

Closed
XbNz opened this issue Dec 9, 2024 · 4 comments
Closed

queue:work is not persistent #443

XbNz opened this issue Dec 9, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@XbNz
Copy link
Contributor

XbNz commented Dec 9, 2024

What were you trying to do?

.

What happened?

.

How to reproduce the bug

.

Package Versions

        "nativephp/electron": "0.8.7",
        "nativephp/laravel": "dev-main",
        "nativephp/php-bin": "0.5.5",

PHP Version

8.3

Laravel Version

v11.34.2

Node Version

v23.3.0

Which operating systems have you seen this occur on?

No response

OS version

macOS 15.1

Notes

I tried to fix this one myself, but I think it might be beneficial to completely rethink the queue system in light of the child process rollout.

I'm not as familiar with the javascript side of NativePHP, but I think queue workers are launched here:

function startQueueWorker(secret, apiPort, phpIniSettings = {}) {
    const env = getDefaultEnvironmentVariables(secret, apiPort);

    const phpOptions = {
        cwd: appPath,
        env
    };

    return callPhp(['artisan', 'queue:work', '-q'], phpOptions, phpIniSettings);
}

If that's the case, there is no way this alone can be persistent. Correct me if I'm wrong, but if a process dies, it will require a full relaunch to resolve it.

Due to the memory intensive nature of the application I'm working on, queue workers are running out of memory and exiting with exit code 12. This was not being properly reported by the native:serve dev environment, so it actually took me a little digging to even realize that the workers are crashing with exit code 12. Before learning this, from my perspective, it seemed as though the workers were arbitrarity ceasing to work.

Now, obviously for my case it is better to fix my code so it stops crashing the queue workers. But I think there might be a better solution for the way we handle queue workers as well.

To debug the crashing workers, I completely removed the queue:work stuff from the php.ts file (replaced them with artisan:inspire lol), and launched the workers myself from a random livewire action:

public function queue(): void
{
    ChildProcess::artisan(['queue:work'], 'test-worker', persistent: true);
}

This works like a charm. Almost.

Because we're handling the queue in a child process, every time the queue processes something, it is also considered as the child process receiving a message. The child process related events in Native\Laravel\Events\ChildProcess are broadcasting to the queue ShouldBroadcast vs the ShouldBroadcastNow which is used by all other events NativePHP fires. This results in a never ending stream of broadcasts being sent to the queue worker. Not ideal. If there is no good reason to use ShouldBroadcast, then this is a really simple fix.

I also noticed that the queue worker encounters a database lock when launched from the NativePhpServiceProvider::class, which is why I'm launching it from a livewire method. This also needs to be resolved.

Aside from that, I really think child processes are ready to handle queue as well. At the very least, we'll get the advantage of being able to launch multiple queue workers.

@XbNz XbNz added the bug Something isn't working label Dec 9, 2024
@simonhamp
Copy link
Member

simonhamp commented Dec 20, 2024

it might be beneficial to completely rethink the queue system in light of the child process rollout.

I completely agree!

If that's the case, there is no way this alone can be persistent. Correct me if I'm wrong, but if a process dies, it will require a full relaunch to resolve it.

Correct. We don't currently ensure the queue worker remains alive. Moving to a peristent ChildProcess would solve this.

If there is no good reason to use ShouldBroadcast, then this is a really simple fix.

The difference between ShouldBroadcast and ShouldBroadcastNow is that ShouldBroadcastNow doesn't also fire the event across Laravel internally, it only broadcasts it out to any broadcasting channels. This means you couldn't have a Laravel-side listener respond to the event as it occurred - it would have to go all the way across to Electron and back first.

Needs testing here before making this change as I'm not 100% sure if we're relying on this behaviour internally, but in principle I'm not against it, it may just close one little door of potential efficiency.

At the very least, we'll get the advantage of being able to launch multiple queue workers.

Yeh I would love this. It would be great to have some flexibility in the config around this as I think that would take the maturity of the queue system to a new level.

I'm imagining something like this in the config/nativephp.php where we allow the developer to configure their queue workers using the same parameters they'd be able to pass to the queue:work Artisan command:

'workers' => [
    ['--queue=default,images', '--memory=64', '--timeout=60'],
    ['--queue=github', '--backoff=60', '--rest=10', '--tries=3'],
],

// Or potentially like this, which feels a bit more "Horizon-like"...

"workers" => [
    [
        'queues' => ['default', 'images'],
        'memory' => 64,
        'timeout' => 60
    ],
    [
        'queues' => 'github',
        'backoff' => 60
        'rest' => 10,
        'tries' => 3
    ],
],

This way, they have precise control over exactly how many queue workers they start and their individual configurations.

What do you think?

Sharing other references in case there are relevant notes there:

@XbNz
Copy link
Contributor Author

XbNz commented Dec 22, 2024

Needs testing here before making this change

Tested it for user-facing behavior. Works well. Here's a test repo:
https://github.com/XbNz/nativephp-child-queues-test

Just do native:serve and click the 4 buttons after installing deps. The event is firing and reaching the parent PHP process as I expected it to. No problems here!

This means you couldn't have a Laravel-side listener respond to the event as it occurred

It seemingly works? I'm not sure if I'm testing precisely what you are referring to, but I think I am. Here's the line I'm talking about: https://github.com/XbNz/nativephp-child-queues-test/blob/650dfe0bbb86ff77dabf6d67066ea887ad2b2361/app/Listeners/SampleListener.php#L14

If the Laravel listener wasn't working, this event should not have been caught by this here. In my testing, I'm getting both alerts.

In terms of the config, the only extra key I can think of is to provide control over the child process alias in case we want to spin down a queue worker from userland manually.

"workers" => [
    [
        + 'child_process_alias' => 'blah'
        'queues' => ['default', 'images'],
        'memory' => 64,
        'timeout' => 60
    ],
],

@simonhamp
Copy link
Member

That's great news!

Agreed, giving the worker a name that can be used as the alias is a great idea. How about we use the array key for this? That way there's guaranteed uniqueness:

'workers' => [
    'blah' => [
        'queues' => ['default', 'images'],
        'memory' => 64,
        'timeout' => 60
    ],
],

@simonhamp
Copy link
Member

Closing as this will be resolved by #450 being released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants