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

Finalize helper on MSHUTDOWN #42

Open
Anilm3 opened this issue Dec 14, 2021 · 1 comment
Open

Finalize helper on MSHUTDOWN #42

Anilm3 opened this issue Dec 14, 2021 · 1 comment
Labels
enhancement New feature or request extension
Milestone

Comments

@Anilm3
Copy link
Collaborator

Anilm3 commented Dec 14, 2021

Description

While the helper process will eventually timeout if the application is finalised, it will probably not do so after a restart, as
the timeout is not intended to cover these scenarios. This might prevent a successful upgrade, although this will be mitigated in the future through #3 and #4.

Suggested solution

  • Signalling the helper process to terminate during MSHUTDOWN should suffice, although the helper process PID is not available to the extension at the moment, but we can obtain this through the SCM_CREDENTIALS on client_init.
  • However, if the process performing MSHUTDOWN does not partake in the request lifecycle it will never really have a client_init, so a better approach is to use fcntl locks instead of flock, which would allow us to get the PID of the helper process by inspecting struct flock, but since fcntl locks are not inherited after fork, it'll require a few more changes.
  • Another slightly more complex solution is to implement a new command which would allow the extension to tell the helper process to shutdown. I'm not sure this is a good solution in general as it'll allow an unverified process to terminate the helper with a limited (but equivalent) set of credentials.
  • An even simpler and quite common solution is to have the final process doing fork/exec actually write it's PID in the lock file. This would allow the process performing MSHUTDOWN to try-lock and if it fails, it can read the PID and send a signal.
@Anilm3 Anilm3 added enhancement New feature or request extension labels Dec 14, 2021
@Anilm3 Anilm3 modified the milestones: v0.3.0, v0.4.0 Dec 14, 2021
@cataphract
Copy link
Contributor

I don't think that this is a workable change, nor that the problem it's trying to solve (the helper outliving the clients) is a big problem. First, the helper will be closed in two common scenarios:

  • inside docker and,
  • when fpm/apache run inside a cgroup (e.g. when fpm/apache are run through a systemd service).

In these two common scenarios, child processes (even if they daemonize) are tracked and closed when the service ends. Additionally, if the helper process is part of another systemd service, we do not want to make it exit. So we should exit only if we launched the helper. The problem is that, in general, we can't rely on MSHUTDOWN for this. First off, we have no guarantee that MSHUTDOWN will be called when a client exits (it could crash or be be killed for instance). More importantly, clients shutdown as a matter of course, for instance when they've served the maximum number of requests. We would then need to keep a counter for the active clients and shutdown when this reaches 0. This is in itself a bit complicated to make race-free, but even then it's not clear to me how this can work in low traffic scenarios in which possibly only one client exists at one time.

The upgrade/downgrade problem can be mitigated, as you said, with the versioning of the lock/socket. We can also be more aggressive with inactivity timeout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request extension
Projects
None yet
Development

No branches or pull requests

2 participants