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

[5.4] Let worker ungracefully exit on memoryExceeded #17302

Conversation

lifeofguenter
Copy link
Contributor

See: #17296

@taylorotwell taylorotwell merged commit a0f9607 into laravel:5.4 Jan 12, 2017
@GrahamCampbell GrahamCampbell changed the title Let worker ungracefully exit on memoryExceeded [5.4] Let worker ungracefully exit on memoryExceeded Jan 12, 2017
@taylorotwell
Copy link
Member

Can you explain more about why you wanted this?

@lifeofguenter
Copy link
Contributor Author

lifeofguenter commented Jan 13, 2017

With this PR we are distinguishing between a caught crash (in this case only due to memory "leaks", but it can be extended surely) and a user desired action. Apart from it being semantically more correct (e.g. a programm error in the console should never exit with 0, which translates to "all good" under linux/unix) it has the following nice side effect:

When doing blue-green deployments you would only be able to stop the old workers by "killing" the processes (supervisorctl stop all), which means that potentially jobs might be aborted before they completed properly.

Now, it would be possible to piggyback on the command php artisan queue:restart and use that instead to gracefully shutdown the queue-workers.

For this to work, you'd only need to configure supervisord with the following settings:

  • autorestart=unexpected (default)
  • exitcodes=0,2(default)

If you want supervisord to actually restart in any case (or in other cases) all combinations are now possible, e.g.

  • autorestart=true would restart the queue-worker (as the php artisan queue:restart command suggests)

(there are surely different ways to solve this problem as well)

Without these changes, you can still have the queue-worker to shutdown "gracefully" by running the command php artisan queue:restart, but it would also gracefully shutdown on a memory leak, which is maybe something that is not desired in all cases.

@GrahamCampbell
Copy link
Member

With this PR we are distinguishing between a caught crash (in this case only due to memory "leaks", but it can be extended surely)

Laravel shutting down due to using too much memory is not a crash, it's a correct exit.

@GrahamCampbell
Copy link
Member

A crash would be if we implement the system to throw an out of memory exception, but it wasn't implemented like that, because running out of memory is not considered an error in this context.

@lifeofguenter
Copy link
Contributor Author

Laravel shutting down due to using too much memory is not a crash, it's a correct exit.

Emphasize is on _ distinguishing_ - it is a exit-code, not a "crash"-code.

Why should Laravel explicitly exit with 0 if it is using too much memory? Why should it not throw an out of memory exception in first place?

@GrahamCampbell
Copy link
Member

Why should Laravel explicitly exit with 0 if it is using too much memory? Why should it not throw an out of memory exception in first place?

Because it's not a crash, it could have continued to use more memory. It is configured to gracefully stop and be restarted by some other process managing it, like supervisor.

@lifeofguenter
Copy link
Contributor Author

Ah interesting. To me it makes more sense for such a behavior of the process starter itself (be it systemd, supervisord, watchdog, etc.) - but in this case the queue-worker is a single process with no "self-watching" or "self-healing" capability (can for example be done if there is a master process that forks itself, while the master process keeps checking on the fork).

In this case it is only a layer controlled by another layer. So how to "signal"/communicate with the other layer properly? Well we can do it with standards.

Think of it as a web-request, php-fpm communicating with the webserver. You would explicitly set a http-500 header on errors (or 404 on route not found) from within the php-fpm layer and not "let the webserver somehow decide over non-standard ways how to interpret the response of php-fpm".

Maybe also it makes more sense if we stop wording it as "crash" but just as it is: status code. Lets make it possible in first place for $program to decide on the action to do according to how the worker exits. Either one can completely ignore the status-code and restart the worker in any case, or it can have a fine granularity on the events.

I really can't stress enough how important it is in a unix/linux to rely on proper return/exit/status codes (http://www.tldp.org/LDP/abs/html/exit-status.html):

Every command returns an exit status (sometimes referred to as a return status or exit code). A successful command returns a 0, while an unsuccessful one returns a non-zero value that usually can be interpreted as an error code. Well-behaved UNIX commands, programs, and utilities return a 0 exit code upon successful completion, though there are some exceptions.

When I run: curl foobar it will output curl: (6) Could not resolve host: foobar with a status-code of 6 (echo $?). Curl most certainly did not crash, but now at least I can programmatically handle the case of curl being unable to resolve a domain :)

@GrahamCampbell
Copy link
Member

No, curl did crash, it had an error. Laravel is not having an issue, it's just done. There's a difference. That's why it's not implemented using an exception.

@lifeofguenter
Copy link
Contributor Author

Laravel is not having an issue, it's just done.

Sure. It's done, because it ran out of memory. The same way curl is done because it had no dns to resolve.

What is the use-case of that implementation anyways? Because either way, if it were an exception or if just exit(0), the queue-worker will stop?

@GrahamCampbell
Copy link
Member

Sure. It's done, because it ran out of memory.

It didn't "run out of memory". It just stopped because it was configured to stop once it had used that much memory. See the difference?

@lifeofguenter
Copy link
Contributor Author

lifeofguenter commented Jan 13, 2017

If something is configured or implemented, that does not change the fact of differentiating exit codes?

Same way I can "configure" curl to exit with a non-zero code:

curl -k https://foobar.de/foobar

vs.

curl -fk https://foobar.de/foobar

See? The implementation or configuration says nothing about an exit code. I can have the webapp "configured" to throw a 404 or it can be thrown implicitly - it's still a 404 :)

What are the advantages of not differentiating status codes and letting both cases exit with 0?

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.

3 participants