Skip to content

Comments

Fix garbage collection of Notifier#96

Open
blueyed wants to merge 1 commit intoseb-m:masterfrom
blueyed:fix-garbage-collect-notifier
Open

Fix garbage collection of Notifier#96
blueyed wants to merge 1 commit intoseb-m:masterfrom
blueyed:fix-garbage-collect-notifier

Conversation

@blueyed
Copy link
Contributor

@blueyed blueyed commented Mar 16, 2015

This uses a weak reference to the notifier in _SysProcessEvent and adds
a __del__ method for Notifier.

This prevents pyinotify from leaking inotify file descriptors
(instances) and its watches.

Fixes #95

I have used the following script to test it:

from __future__ import print_function

import gc
import os

from pyinotify import Notifier, WatchManager

def has_inotify_fds():
    "Does the current process have inotify fds?"
    r = 0
    for x in os.walk('/proc/{}/fd'.format(os.getpid())):
        for y in x[2]:
            path = os.path.join(x[0], y)
            if os.path.exists(path):
                if 'inotify' in os.path.join(os.readlink(path)):
                    r += 1
    return r

gc.disable()

wm = WatchManager()
n = Notifier(wm)
assert has_inotify_fds() == 1

# Stopping multiple times should work:
n.stop()
assert has_inotify_fds() == 0
n.stop()

gc.collect()
# XXX: needs a new WatchManager, because the previous fd gets closed by stop!
wm = WatchManager()
n = Notifier(wm)
assert has_inotify_fds() == 1

# Collect anything.
gc.collect()
assert n in gc.get_objects()

n = None
assert gc.garbage == []
assert not any(isinstance(x, Notifier) for x in gc.garbage)
assert not any(isinstance(x, Notifier) for x in gc.get_objects())

assert has_inotify_fds() == 0

This uses a weak reference to the notifier in _SysProcessEvent and adds
a `__del__` method for Notifier.

This prevents pyinotify from leaking inotify file descriptors
(instances) and its watches.

Fixes seb-m#95

I have used the following script to test it:

    from __future__ import print_function

    import gc
    import os

    from pyinotify import Notifier, WatchManager

    def has_inotify_fds():
        "Does the current process have inotify fds?"
        r = 0
        for x in os.walk('/proc/{}/fd'.format(os.getpid())):
            for y in x[2]:
                path = os.path.join(x[0], y)
                if os.path.exists(path):
                    if 'inotify' in os.path.join(os.readlink(path)):
                        r += 1
        return r

    gc.disable()

    wm = WatchManager()
    n = Notifier(wm)
    assert has_inotify_fds() == 1

    # Stopping multiple times should work:
    n.stop()
    assert has_inotify_fds() == 0
    n.stop()

    gc.collect()
    # XXX: needs a new WatchManager, because the previous fd gets closed by stop!
    wm = WatchManager()
    n = Notifier(wm)
    assert has_inotify_fds() == 1

    # Collect anything.
    gc.collect()
    assert n in gc.get_objects()

    n = None
    assert gc.garbage == []
    assert not any(isinstance(x, Notifier) for x in gc.garbage)
    assert not any(isinstance(x, Notifier) for x in gc.get_objects())

    assert has_inotify_fds() == 0
@blueyed blueyed force-pushed the fix-garbage-collect-notifier branch from 89a88f6 to c88f370 Compare June 6, 2015 11:00
@blueyed
Copy link
Contributor Author

blueyed commented Jun 6, 2015

Rebased on master.

@seb-m
What do you think about it?

@seb-m
Copy link
Owner

seb-m commented Jun 6, 2015

Hi @blueyed,

I still don't have an opinion about it.

I must say I didn't have time to review it and think about it carefully and this is the kind of structural change I don't want to commit lightly.

@phillipberndt
Copy link

FYI, I just ran into this issue and thought of a similar fix. But I think that closing the fd from Notifier is the wrong thing to do (and has been in the first place, it's not introduced by this PR); the fd is owned by WatchManager, hence it should not be closed until the WatchManager is destroyed. I'd therefore suggest an additional change: Remove the call to os.close from Notifier.stop, and add a WatchManager.__del__ method that calls WatchManager.close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Provide hint when running out of watches / instances; leak via react.py

3 participants