ZConfig

Merge lp:~lifeless/zconfig/bug-481512 into lp:~fdrake/zconfig/trunk

  1. bug-481512
  2. Merge into trunk
Proposed by Robert Collins
Status: Needs review
Proposed branch: lp:~lifeless/zconfig/bug-481512
Merge into: lp:~fdrake/zconfig/trunk
Diff against target: 100 lines (+41/-33)
2 files modified
ZConfig/components/logger/loghandler.py (+41/-16)
ZConfig/components/logger/tests/test_logger.py (+0/-17)
To merge this branch: bzr merge lp:~lifeless/zconfig/bug-481512
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Fred Drake Pending
Review via email: mp+122612@code.launchpad.net

Description of the change

This is an alternative fix for the signal handling race condition.

It:
 - sets a flag atomically, by binding a name to True.
 - does a flush of the stream
 - changes the reopen and close orders to prevent self.stream being a closed file.

To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

Hi Robert.

I think the code and safe and okay to land. Before you do land, Review the comment in line 51+. I think line 55 is from an earlier edit and it does not make sense.

review: Approve (code)
Reply

Unmerged revisions

503. By Robert Collins

Flush, hopefully safely.

502. By Robert Collins

Another approach for dealing with race conditions in reopen when used from signal handlers.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ZConfig/components/logger/loghandler.py'
2--- ZConfig/components/logger/loghandler.py 2012-02-11 06:04:50 +0000
3+++ ZConfig/components/logger/loghandler.py 2012-09-04 05:45:23 +0000
4@@ -69,27 +69,52 @@
5 self.baseFilename = filename
6 self.mode = mode
7 self._wr = weakref.ref(self, _remove_from_reopenable)
8+ self._reopen = False
9 _reopenable_handlers.append(self._wr)
10
11 def close(self):
12- self.stream.close()
13- # This can raise a KeyError if the handler has already been
14- # removed, but a later error can be raised if
15- # StreamHandler.close() isn't called. This seems the best
16- # compromise. :-(
17- try:
18- StreamHandler.close(self)
19- except KeyError:
20- pass
21- _remove_from_reopenable(self._wr)
22-
23- def reopen(self):
24- self.acquire()
25- try:
26+ try:
27+ _remove_from_reopenable(self._wr)
28+ finally:
29 self.stream.close()
30+ # This can raise a KeyError if the handler has already been
31+ # removed, but a later error can be raised if
32+ # StreamHandler.close() isn't called. This seems the best
33+ # compromise. :-(
34+ try:
35+ StreamHandler.close(self)
36+ except KeyError:
37+ pass
38+
39+ def acquire(self):
40+ super(FileHandler, self).acquire()
41+ if self._reopen:
42+ # Ensure that self.stream is never assigned-but-closed.
43+ old_stream = self.stream
44 self.stream = open(self.baseFilename, self.mode)
45- finally:
46- self.release()
47+ self._reopen = False
48+ old_stream.close()
49+
50+ def reopen(self):
51+ # Note: this may be called from a signal handler, so it cannot:
52+ # - take non reentrant locks
53+ # which includes locks written in python that are built on
54+ # non-reentrant C primitives
55+ # make any assumptions about state.
56+ # -> we set a flag, which is a simple assignment, and that is inspected
57+ # by acquire.
58+ # Any in-progress log message (that already holds the lock) will log to
59+ # the stream that was open when it started.
60+ # The next log message will cause an actual re-open to occur.
61+ # This means that there is an unknown, and unknownable duration between
62+ # reopen() being called, and the reopen happening.
63+ # If two signals are received in short order, its possible that the
64+ # reset in acquire will be overwritten, but the absence of a
65+ # compare-exchange, and the inability to use Python based locks in a
66+ # signal handler makes this intractable.
67+ # See http://bugs.python.org/issue13697 for more information.
68+ self._reopen = True
69+ self.stream.flush()
70
71
72 class Win32FileHandler(FileHandler):
73
74=== modified file 'ZConfig/components/logger/tests/test_logger.py'
75--- ZConfig/components/logger/tests/test_logger.py 2012-02-11 20:34:47 +0000
76+++ ZConfig/components/logger/tests/test_logger.py 2012-09-04 05:45:23 +0000
77@@ -628,23 +628,6 @@
78 logger.removeHandler(handler)
79 handler.close()
80
81- def test_filehandler_reopen_thread_safety(self):
82- # The reopen method needs to do locking to avoid a race condition
83- # with emit calls. For simplicity we replace the "acquire" and
84- # "release" methods with dummies that record calls to them.
85-
86- fn = self.mktemp()
87- h = self.handler_factory(fn)
88-
89- calls = []
90- h.acquire = lambda: calls.append("acquire")
91- h.release = lambda: calls.append("release")
92-
93- h.reopen()
94- h.close()
95-
96- self.assertEqual(calls, ["acquire", "release"])
97-
98
99 def test_logger_convenience_function_and_ommiting_name_to_get_root_logger():
100 """

Subscribers

People subscribed via source and target branches