Opened 12 years ago

Closed 11 years ago

#2483 defect closed fixed (fixed)

Undesirable garbage collector/spawnProcess interaction

Reported by: Jean-Paul Calderone Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: therve Branch:
Author:

Description

Finalizers can run in the child process created by spawnProcess:

from os import getpid

from twisted.internet import reactor
from twisted.internet.utils import getProcessOutputAndValue
from twisted.internet.task import LoopingCall

pid = getpid()
print pid

class BadObject(object):
    def __del__(self):
        if getpid() != pid:
            print getpid(), '!=', pid


class CycleParticipant(object):
    pass


def makeSomeGarbage():
    a = CycleParticipant()
    b = CycleParticipant()
    a.b = b
    b.a = a
    a.bad = BadObject()


def runChildProcess():
    makeSomeGarbage()
    d = getProcessOutputAndValue("/bin/false")
    def cbOutput((out, err, code)):
        if out or err:
            print 'Unexpected:', out, err
    d.addCallback(cbOutput)
    return d


def main():
    d = LoopingCall(runChildProcess)
    d.start(0).addErrback(lambda ign: reactor.stop())
    reactor.run()

if __name__ == '__main__':
    main()

That they can run there at all is bad and probably surprising to most people. As a specific wrinkle, they can run either before the file descriptors have been hooked up to the parent, in which case the output will just go to the parent's output (resulting in double output, since the finalizer will presumably run and do the same thing in the parent), or afterwards, in which case the parent will actually /see/ output notionally (but not actually) generated by itself as coming from the child, or it can happen while the file descriptors are being set up, in which case some random unpredictable thing will happen.

Change History (11)

comment:1 Changed 12 years ago by Jean-Paul Calderone

I came across this because of a failure on one of the buildslaves. twisted.test.test_process.ProcessTestCase.testManyProcesses failed because it saw the output from numerous temporary file objects being garbage collected (and failing) in the output from its child process.

This poorly written test reported this in the log file:

twisted.test.test_process.ProcessTestCase.testManyProcesses ... err != 1234: "Exception exceptions.IOError: (9, 'Bad file descriptor') in <bound method _TemporaryFileWrapper.__del__ of <closed file '<fdopen>', mode 'w+b' at 0xad51dbf0>> ignored\nException exceptions.IOError: (9, 'Bad file descriptor') in <bound method _TemporaryFileWrapper.__del__ of <closed file '<fdopen>', mode 'w+b' at 0xad51dba8>> ignored\nException exceptions.IOError: (9, 'Bad file descriptor') in <bound method _TemporaryFileWrapper.__del__ of <closed file '<fdopen>', mode 'w+b' at 0xad51dc80>> ignored\nException exceptions.IOError: (9, 'Bad file descriptor') in <bound method _TemporaryFileWrapper.__del__ of <closed file '<fdopen>', mode 'w+b' at 0xad51dd10>> ignored\nException exceptions.IOError: (9, 'Bad file descriptor') in <bound method _TemporaryFileWrapper.__del__ of <closed file '<fdopen>', mode 'w+b' at 0xad51dd58>> ignored\n1234"

comment:2 Changed 12 years ago by jknight

So, gc.disable() before forking?

comment:3 Changed 12 years ago by Jean-Paul Calderone

That's the only thing that's come to mind so far. I guess it's not really very bad.

comment:4 Changed 11 years ago by Jean-Paul Calderone

Owner: changed from Glyph to Jean-Paul Calderone
Status: newassigned

comment:5 Changed 11 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted
Priority: normalhighest
Status: assignednew

I added the necessary gc calls, and tests. Code is in spawnprocess-gc-2483. I feel kind of bad about this code, but I'm not totally sure why. Probably because the spawnProcess code is so hairy, complex, undertested, and still buggy. After this is merged I'll fix some of the other bugs nearby, like leaked pipes and incorrect set{u,g}id calls.

comment:6 Changed 11 years ago by therve

Cc: therve added
Keywords: review removed
Owner: set to Jean-Paul Calderone

test_mockWithForkErrorGarbageCollectorDisabled docstring is talking about the 'enable' case, it should be corrected.

Otherwise, everything looks fine, except I don't see a real test for the actual problem.

I'm relatively convinced it fixes the reported behavior, but having a deterministic test to show it would be great. I think it's probably possible with a forced gc.collect() somewhere (maybe simply mocking _setupChild or _execChild ?).

comment:7 Changed 11 years ago by Jean-Paul Calderone

(In [21683]) docstring cleanups

refs #2483

comment:8 Changed 11 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

Fixed the docstring. Re-reading it, I found some other bits of the wording confusing, so I changed slightly more than the enable/disable mistake.

I still can't think of any better gc test to write. As discussed on IRC, aside from checking to make sure it is disable at the right times, I can't think of any way to test that the garbage collector isn't invoked in the forked child.

comment:9 Changed 11 years ago by therve

Keywords: review removed
Owner: set to Jean-Paul Calderone

Please merge.

comment:10 Changed 11 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [21686]) Merge spawnprocess-gc-2483

Author: exarkun Reviewer: therve Fixes #2483

Disable garbage collection completely in the forked child used to implement IReactorProcess.spawnProcess on POSIX to prevent finalizers or weakref callbacks from executing in that process.

comment:11 Changed 8 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.