Skip to content

Commit

Permalink
Close INotify filedescriptor if a directory is unwatched due to fs de…
Browse files Browse the repository at this point in the history
…letion

INotify will transparently unwatch a directory if it has been deleted from
the filesystem. Not making a call to .loseConnection() results in
leaving permanently opened [orphaned] file descriptors.

We now close that file descriptor since it can no longer trigger.
  • Loading branch information
ChaoticMind committed Mar 16, 2020
1 parent b60f198 commit 073da70
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 0 deletions.
1 change: 1 addition & 0 deletions src/twisted/internet/inotify.py
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,7 @@ def _doRead(self, in_):
self._addChildren, self._watchpoints[new_wd])
if mask & IN_DELETE_SELF:
self._rmWatch(wd)
self.loseConnection()


def _addChildren(self, iwp):
Expand Down
32 changes: 32 additions & 0 deletions src/twisted/internet/test/test_inotify.py
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,38 @@ def _eb():
return d


def test_deleteSelfLoseConnection(self):
"""
L{inotify.INotify} closes the file descriptor after removing a
directory from the filesystem (and therefore from the watchlist).
"""
def cbNotified(result):
def _():
try:
self.assertFalse(self.inotify._isWatched(self.dirname))
self.assertFalse(self.inotify.connected)
d.callback(None)
except Exception:
d.errback()

(ignored, filename, events) = result
self.assertEqual(filename.asBytesMode(), self.dirname.asBytesMode())
self.assertTrue(events & inotify.IN_DELETE_SELF)
reactor.callLater(0, _)

self.assertTrue(
self.inotify.watch(
self.dirname, mask=inotify.IN_DELETE_SELF,
callbacks=[lambda *args: notified.callback(args)]))

notified = defer.Deferred()
notified.addCallback(cbNotified)

self.dirname.remove()
d = defer.Deferred()
return d


def test_ignoreDirectory(self):
"""
L{inotify.INotify.ignore} removes a directory from the watchlist
Expand Down
1 change: 1 addition & 0 deletions src/twisted/newsfragments/9777.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
INotify will close its file descriptor if a directory is automatically removed by twisted from the watchlist because it's deleted, avoiding orphaned filedescriptors.

4 comments on commit 073da70

@wesleyvanderree
Copy link

Choose a reason for hiding this comment

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

Hi, I have a question about this patch, Maybe I lack understand the inner workings of Inotify but shoulnd't you only call loseConnection when you don't have any watchpoints anymore?

for example i build a small test script:

#!/usr/bin/env python3

from twisted.internet import reactor
from twisted.internet import inotify
from twisted.python import filepath

notifier = inotify.INotify()
notifier.startReading()

def onchange(_, path, ___):
    print("Change {}".format(path))
    return True

notifier.watch(filepath.FilePath("/tmp/test/1"), mask=(inotify.IN_CREATE | inotify.IN_DELETE | inotify.IN_CLOSE_WRITE | inotify.IN_MOVED), autoAdd=False, recursive=False, callbacks=[onchange])
notifier.watch(filepath.FilePath("/tmp/test/2"), mask=(inotify.IN_CREATE | inotify.IN_DELETE | inotify.IN_CLOSE_WRITE | inotify.IN_MOVED), autoAdd=False, recursive=False, callbacks=[onchange])

reactor.run()

when I run this script I'm interested in changes from both the "/tmp/test/1" and the "/tmp/test/2" directory. When somebody removes the "/tmp/test/1" directory I never get updates from the "/tmp/test/2" directory anymore because the connection is closed.

mkdir -p /tmp/test/1; mkdir -p /tmp/test/2
rm -r /tmp/test/1
rm -r /tmp/test/2

would produce:
python3 ./test.py
Change: FilePath(b'/tmp/test/1')

the removal of the second dir is never notified.

When I override the "loseConnection" and only close connections when i don't have any watchpoints anymore I do get a notify of the second directory removal:

#!/usr/bin/env python3

from twisted.internet import reactor
from twisted.internet import inotify
from twisted.python import filepath

class TestInotify(inotify.INotify):

    def loseConnection(self):
        if not self._watchpoints:
            super(TestInotify, self).loseConnection()
        else:
            print("Still {} watchpoints, not closing connection.".format(len(self._watchpoints)))


notifier = TestInotify()
notifier.startReading()

def onchange(_, path, ___):
    print("Change: {}".format(path))
    return True

notifier.watch(filepath.FilePath("/tmp/test/1"), mask=(inotify.IN_CREATE | inotify.IN_DELETE | inotify.IN_CLOSE_WRITE | inotify.IN_MOVED), autoAdd=False, recursive=False, callbacks=[onchange])
notifier.watch(filepath.FilePath("/tmp/test/2"), mask=(inotify.IN_CREATE | inotify.IN_DELETE | inotify.IN_CLOSE_WRITE | inotify.IN_MOVED), autoAdd=False, recursive=False, callbacks=[onchange])

reactor.run()

python3 ./test.py
Change: FilePath(b'/tmp/test/1')
Change: FilePath(b'/tmp/test/2')

I hope I explained it clearly.

Kind regards,

Wesley

@exarkun
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for pointing this out. The change here does not look valid to me. I don't know if anyone else regularly monitors commit comments. The issue tracker, mailing list, or IRC channel are probably better forums for this kind of discussion. I left a comment with a link to this on the original issue, though, and I'll try to follow up in a couple days if no one else jumps on it.

@ChaoticMind
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it does introduce a bug! More discussion in the associated issue.

Thank you for catching it and tracking it down, I know it can be a pain.

@wesleyvanderree
Copy link

Choose a reason for hiding this comment

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

No worries, troubleshooting can also be fun, and it had to be fixed in my use case anyway.

thanks for the response!

Wesley

Please sign in to comment.