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

Typo fix for reexec related environ variables #2559

Merged
merged 1 commit into from
Aug 10, 2024

Conversation

laggardkernel
Copy link
Contributor

@laggardkernel laggardkernel commented Apr 8, 2021

Commit 1 is a refactor. The others are reexec environ related.

### Commit 1
Moved Arbiter.log.close_on_exec() from Arbiter.init_signals() to Arbiter.setup(). Seem more appropriate.

### Commit 2
In Arbiter.maybe_promote_master(), cleanup more env variables passed from Arbiter.reexec() called in parent.

Considering whether to unset LISTEN_PID, LISTEN_FDS is handled by systemd.listen_fds(unset_environment=True) under Arbiter.start(). I leave these two env vars alone.

Commit 3

In Arbiter.reexec(), GUNICORN_FD may not be passed to the new master process,

    def reexec(self):
        ...
        self.cfg.pre_exec(self)
        # CO(lk): use env vars to pass pid, fds to the new `reexec`ed child process
        environ = self.cfg.env_orig.copy()
        environ['GUNICORN_PID'] = str(master_pid)

        if self.systemd:
            environ['LISTEN_PID'] = str(os.getpid())
            environ['LISTEN_FDS'] = str(len(self.LISTENERS))
        else:
            environ['GUNICORN_FD'] = ','.join(
                str(l.fileno()) for l in self.LISTENERS)

        os.chdir(self.START_CTX['cwd'])
        # exec the process using the original environment
        os.execvpe(self.START_CTX[0], self.START_CTX['args'], environ)

Replace GUNICORN_FD with GUNICORN_PID instead to detect whether a process is a reexeced process.

    def setup(self, app):
        # reopen files
        if 'GUNICORN_PID' in os.environ:
            self.log.reopen_files()

@benoitc benoitc self-assigned this Jun 9, 2021
@benoitc
Copy link
Owner

benoitc commented Jun 18, 2021

which bug are you trying to fix? I am not sure about the reasonning there. Can you elaborate?

`GUNICORN_FD` is not always set in a `reexec`ed child process.
Considering `Arbiter.master_pid` is set after log reopening,
choose `GUNICORN_PID`.
@laggardkernel laggardkernel changed the title Bugfix for reexec related environ variables Typo fix for reexec related environ variables Jun 22, 2021
@laggardkernel
Copy link
Contributor Author

laggardkernel commented Jun 22, 2021

Sorry for making a fuss here. The PR is a trivial typo fix, not a serious bug.
After spending some time re-reading the source code, I dropped unrelated commits
and only keeps the GUNICORN_* environment variable usage.

In commit 67bd75f, you avoided to do self.log.reopen_files() on initial startup,
but only do it for new child arbiter created from old arbiter by calling Arbiter.reexec().
The problem is that env variable GUNICORN_FD is not enough to detect the new arbiter.
GUNICORN_FD is not set for Arbiter started by systemd. What we should use
is GUNICORN_PID.

class Arbiter(object):
    ...
    def setup(self, app):
        ...
        # reopen files
-        if 'GUNICORN_FD' in os.environ:
+        if 'GUNICORN_PID' in os.environ:
            self.log.reopen_files()
        ...

This could be confirmed in Arbiter.reexec()

    def reexec(self):
        ...
        environ = self.cfg.env_orig.copy()
        environ['GUNICORN_PID'] = str(master_pid)

        if self.systemd:
            environ['LISTEN_PID'] = str(os.getpid())
            environ['LISTEN_FDS'] = str(len(self.LISTENERS))
        else:
            environ['GUNICORN_FD'] = ','.join(
                str(l.fileno()) for l in self.LISTENERS)
        ...
        os.execvpe(self.START_CTX[0], self.START_CTX['args'], environ)

Apologize again for wasting your time caused by exaggerated word "Bugfix" in my title.

@pajod pajod mentioned this pull request Dec 10, 2023
@benoitc benoitc merged commit 334392e into benoitc:master Aug 10, 2024
@benoitc
Copy link
Owner

benoitc commented Aug 10, 2024

thanks for the change!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants