[Zope-Checkins] SVN: Zope/trunk/ - The ZEO server now records its PID to a file like the ZEO

[Chris McDonough]

Some suggestions (I'd be happy to do them if you don't have time, and
you agree, Sidnei):

- don't rely on INSTANCE_HOME and just make the pid_filename
  ZConfig setting "required".

- Dont even bother trying to call make_pidfile or
  remove_pidfile if we detect we're on Windows.

- Write the pidfile even if we're in readonly mode (I suspect
  you lifted this code from Zope, which also maybe does the
  wrong thing here, depending on your definition of "readonly").


On Thu, 2005-03-17 at 14:50, Tim Peters wrote:
> [Sidnei da Silva]
> > Log message for revision 29523:
> > 
> >        - The ZEO server now records its PID to a file like the ZEO
> >          client. Defaults to /var/ZEO.pid, and its
> >          configurable in /etc/zeo.conf.
> 
> Sidnei, I don't want to merge this into ZODB before some seeming
> problems get addressed (note that if I don't merge it into ZODB, the
> changes here will simply vanish the next time ZODB gets stitched into
> the Zope tree):
> 
> ...
> 
> > Modified: Zope/trunk/lib/python/ZEO/runzeo.py
> > ===================================================================
> > --- Zope/trunk/lib/python/ZEO/runzeo.py 2005-03-17 12:12:01 UTC (rev 29522)
> > +++ Zope/trunk/lib/python/ZEO/runzeo.py 2005-03-17 12:14:32 UTC (rev 29523)
> > @@ -104,6 +104,8 @@
> >                  None, 'auth-database=')
> >         self.add('auth_realm', 'zeo.authentication_realm',
> >                  None, 'auth-realm=')
> > +        self.add('pid_file', 'zeo.pid_filename',
> > +                 None, 'pid-file=')
> > 
> > class ZEOOptions(ZDOptions, ZEOOptionsMixin):
> > 
> > @@ -126,6 +128,7 @@
> >         self.setup_default_logging()
> >         self.check_socket()
> >         self.clear_socket()
> > +        self.make_pidfile()
> >         try:
> >             self.open_storages()
> >             self.setup_signals()
> > @@ -134,6 +137,7 @@
> >         finally:
> >             self.close_storages()
> >             self.clear_socket()
> > +            self.remove_pidfile()
> > 
> >     def setup_default_logging(self):
> >         if self.options.config_logger is not None:
> > @@ -228,6 +232,37 @@
> >         #     Should we restart as with SIGHUP?
> >         log("received SIGUSR2, but it was not handled!", level=logging.WARNING)
> > 
> > +    def make_pidfile(self):
> > +        if not self.options.read_only:
> 
> Why is an exception made for read_only?
> 
> > +            pidfile = self.options.pid_file
> > +            # 'pidfile' is marked as not required.
> > +            if not pidfile:
> > +                pidfile = os.path.join(os.environ["INSTANCE_HOME"],
> > +                                       "var", "ZEO.pid")
> 
> If INSTANCE_HOME isn't defined in the environment, this will raise
> KeyError, and runzeo.py will die.  This will in fact be the case for
> any installation of ZEO that's not running Zope (ZEO has no dependence
> on Zope, except for the one introduced here, and lots of people run
> ZEO who don't run Zope).  Perhaps it should default to the current
> directory if not specified and INSTANCE_HOME isn't defined either.
> 
> > +            try:
> > +                if os.path.exists(pidfile):
> > +                    os.unlink(pidfile)
> > +                pid = os.getpid()
> > +                f = open(pidfile,'w')
> 
> I'm not sure what the pidfile is used for.  Depending on the intended
> use, it may (or may not) be better to open the file in binary mode. 
> Can't guess.
> 
> > +                f.write(`pid`)
> 
> Please don't use backticks -- they're very hard to read, and will go
> away in a future Python.  Use str(pid) instead.
> 
> > +                f.close()
> > +            except IOError:
> > +                error("PID file '%s' cannot be opened.")
> 
> There's no function named error() in runzeo.py.  So if this ever
> triggered, it would actually die with NameError.
> 
> If there _were_ a function named error(), then it would literally
> display '%s', since no file path was interpolated into the error
> string.
> 
> > +            except AttributeError:
> > +                pass  # getpid not supported. Unix/Win only
> 
> This one baffled me.  Are you specifically trying to catch
> AttributeError on, and only on, the "pid = os.getpid()" line 7 lines
> above it?  If so, it would make better sense to check that directly,
> instead of assuming than any AttributeError raised in the whole block
> must have come from that specific line.
> 
> > +    def remove_pidfile(self):
> > +        if not self.options.read_only:
> > +            pidfile = self.options.pid_file
> > +            if not pidfile:
> > +                pidfile = os.path.join(os.environ["INSTANCE_HOME"],
> > +                                       "var", "ZEO.pid")
> 
> Same as above.  It would be much better for make_pidfile() to save the
> path it eventually settled on (or maybe None if it couldn't find a
> workable path) in an instance variable, and have remove_pidfile just
> (re)use that instead of trying to (re)deduce it.
> 
> > +            try:
> > +                if os.path.exists(pidfile):
> > +                    os.unlink(pidfile)
> > +            except IOError:
> > +                error("PID file '%s' could not be removed.")
> 
> Same comments as above about the non-existent error() function, and
> its incomplete argument.
>