app()->terminating() not (always) called with queue:work

[fideloper]

I'm investigating doing some cleanup on some temp files while not relying on __destruct() (which sometimes is called too early).

I found the following. It's not necessarily an issue but was interesting/a bit inconsistent.

I have a queue job that registers a callback with app()->terminating():

app()->terminating(function() {
    Log::info("{$this->job->getJobId()} is running on app termination");
});

Here's what I found:

• ✅ queue:listen gets a log line for every job, which makes sense as queue:listen restarts the framework for each job
• ✅ queue:work --once gets a log line for every job since it fires once and then allows the framework to stop (no while (true) loop)
• ❌queue:work - Does NOT get a log line, ever. This is because Illuminate\Queue\Worker uses exit($status) to stop the worker in all cases (graceful shutdown and ungraceful). This stops the framework from doing regular shutdown behavior.

This is obviously not an issue for most, but since I ran into it, I wanted to see if it was worth changing.

The most obvious solution would be to call app()->terminate() just before exiting. This seems to be exactly what happens for web requests, so it doesn't feel like it would miss too much in terms of shutdown behavior.

Note that the Console Kernel object also calls app()->terminate() so all other artisan calls are terminating the app.

It's really just queue:work that's circumventing normal behavior. Perhaps the queue worker could stop by break'ing out of the while (true) loop instead of exiting.

There could be issues of exiting with the correct status code there, so another idea is to call app()->terminate() just before calling exit($status);

[taylorotwell]

Seems like a reasonable change.

[fideloper]

I’ll try for a PR this afternoon (ish) 👍🏻

…

On Tue, May 12, 2020 at 10:52 Taylor Otwell ***@***.***> wrote: Seems like a reasonable change. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#32779 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADSDU4P7JSXTA43PKMG64TRRFWB7ANCNFSM4M62GCEA> .

[david-kristin]

Hi, @fideloper, any progress on this one?

[fideloper]

You can see the (not-merged) PR here: #32889 and some related PR's that were later merged in that thread.

[fideloper]

(Specifically, this PR was later merged: #32889 )