gunicorn icon indicating copy to clipboard operation
gunicorn copied to clipboard

Typo fix for reexec related environ variables

Open laggardkernel opened this issue 4 years ago • 2 comments
trafficstars

~~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()

laggardkernel avatar Apr 08 '21 07:04 laggardkernel

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

benoitc avatar Jun 18 '21 12:06 benoitc

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.

laggardkernel avatar Jun 22 '21 04:06 laggardkernel

thanks for the change!

benoitc avatar Aug 10 '24 09:08 benoitc