Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

re-implement daemonize #8011

Merged
merged 3 commits into from
Aug 4, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/8011.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Replace daemonize library with a local implementation.
16 changes: 3 additions & 13 deletions synapse/app/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

import gc
import logging
import os
Expand All @@ -22,7 +21,6 @@
import traceback
from typing import Iterable

from daemonize import Daemonize
from typing_extensions import NoReturn

from twisted.internet import defer, error, reactor
Expand All @@ -34,6 +32,7 @@
from synapse.crypto import context_factory
from synapse.logging.context import PreserveLoggingContext
from synapse.util.async_helpers import Linearizer
from synapse.util.daemonize import daemonize_process
from synapse.util.rlimit import change_resource_limit
from synapse.util.versionstring import get_version_string

Expand Down Expand Up @@ -129,17 +128,8 @@ def run():
if print_pidfile:
print(pid_file)

daemon = Daemonize(
app=appname,
pid=pid_file,
action=run,
auto_close_fds=False,
verbose=True,
logger=logger,
)
daemon.start()
else:
run()
daemonize_process(pid_file, logger)
run()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've realised that the original code wrapped this with a try/except for a reason, which is that after stderr gets redirected to /dev/null, any uncaught exceptions will otherwise get thrown away.

I'm currently wondering how best to solve that.



def quit_with_error(error_string: str) -> NoReturn:
Expand Down
1 change: 0 additions & 1 deletion synapse/python_dependencies.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@
"pyyaml>=3.11",
"pyasn1>=0.1.9",
"pyasn1-modules>=0.0.7",
"daemonize>=2.3.1",
"bcrypt>=3.1.0",
"pillow>=4.3.0",
"sortedcontainers>=1.4.4",
Expand Down
120 changes: 120 additions & 0 deletions synapse/util/daemonize.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
# -*- coding: utf-8 -*-
# Copyright (c) 2012, 2013, 2014 Ilya Otyutskiy <[email protected]>
# Copyright 2020 The Matrix.org Foundation C.I.C.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

import atexit
import fcntl
import logging
import os
import signal
import sys


def daemonize_process(pid_file: str, logger: logging.Logger, chdir: str = "/") -> None:
"""daemonize the current process

This calls fork(), and has the main process exit. When it returns we will be
running in the child process.
"""

# If pidfile already exists, we should read pid from there; to overwrite it, if
# locking will fail, because locking attempt somehow purges the file contents.
if os.path.isfile(pid_file):
with open(pid_file, "r") as pid_fh:
old_pid = pid_fh.read()

# Create a lockfile so that only one instance of this daemon is running at any time.
try:
lock_fh = open(pid_file, "w")
except IOError:
print("Unable to create the pidfile.")
sys.exit(1)

try:
# Try to get an exclusive lock on the file. This will fail if another process
# has the file locked.
fcntl.flock(lock_fh, fcntl.LOCK_EX | fcntl.LOCK_NB)
except IOError:
print("Unable to lock on the pidfile.")
# We need to overwrite the pidfile if we got here.
#
# XXX better to avoid overwriting it, surely. this looks racey as the pid file
# could be created between us trying to read it and us trying to lock it.
with open(pid_file, "w") as pid_fh:
pid_fh.write(old_pid)
sys.exit(1)

# Fork, creating a new process for the child.
process_id = os.fork()
Comment on lines +59 to +60
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why the error handling code here was removed from the original library?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basically: I kinda felt that we should be allowing exceptions raised in this function to propagate, rather than suddenly calling sys.exit. It's not particularly clear though. I could be persuaded to go either way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result should be the same either way. Showing the stack trace should give more info, I suppose. 👍


if process_id != 0:
# parent process
sys.exit(0)

# This is the child process. Continue.

# Stop listening for signals that the parent process receives.
# This is done by getting a new process id.
# setpgrp() is an alternative to setsid().
# setsid puts the process in a new parent group and detaches its controlling
# terminal.

os.setsid()
Comment on lines +68 to +74
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this differs from the original code by removing error handling.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os.setsid returns None, so that was dead code.


# point stdin, stdout, stderr at /dev/null
devnull = "/dev/null"
if hasattr(os, "devnull"):
# Python has set os.devnull on this system, use it instead as it might be
# different than /dev/null.
devnull = os.devnull

devnull_fd = os.open(devnull, os.O_RDWR)
os.dup2(devnull_fd, 0)
os.dup2(devnull_fd, 1)
os.dup2(devnull_fd, 2)
os.close(devnull_fd)

# Set umask to default to safe file permissions when running as a root daemon. 027
# is an octal number which we are typing as 0o27 for Python3 compatibility.
os.umask(0o27)

# Change to a known directory. If this isn't done, starting a daemon in a
# subdirectory that needs to be deleted results in "directory busy" errors.
os.chdir(chdir)

try:
lock_fh.write("%s" % (os.getpid()))
lock_fh.flush()
except IOError:
logger.error("Unable to write pid to the pidfile.")
print("Unable to write pid to the pidfile.")
sys.exit(1)

# write a log line on SIGTERM.
def sigterm(signum, frame):
logger.warning("Caught signal %s. Stopping daemon." % signum)
sys.exit(0)

signal.signal(signal.SIGTERM, sigterm)

# Cleanup pid file at exit.
def exit():
logger.warning("Stopping daemon.")
os.remove(pid_file)
sys.exit(0)

atexit.register(exit)

logger.warning("Starting daemon.")