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

[Tim Peters]

[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.