INotify orphaned file descriptor on file/directory deletion

[twisted-trac]

	@ChaoticMind reported
Trac ID	trac#9777
Type	defect
Created	2020-03-16 18:23:46Z

When a directory is watched via twisted's inotify wrappers and is deleted, it's automatically and transparently removed from the watchlist by forcing the catching of inotify.IN_DELETE_SELF.

However, when this occurs, the INotify file descriptor is never closed and will therefore be forever hanging until the twisted process is closed. This could be checked via ls -l /proc/<pid>/fd/.

A workaround to close the file descriptor is to have a callback (passed via watch() that will explicitly catch the mask inotify.IN_DELETE_SELF (even if the user hasn't specified this mask when initiating the watch) and close the file descriptor there (via a call to .loseConnection()).

Searchable metadata

trac-id__9777 9777
type__defect defect
reporter__ChaoticMind ChaoticMind
priority__normal normal
milestone__None None
branch__ 
branch_author__ 
status__closed closed
resolution__fixed fixed
component__core core
keywords__None None
time__1584383026719942 1584383026719942
changetime__1587544903563993 1587544903563993
version__None None
owner__glyph glyph

[twisted-trac]

	@ChaoticMind commented

Link to PR: #1231

[twisted-trac]

	@glyph set owner to @glyph

Review on github. Not a lot to comment on, just a good fix!

[twisted-trac]

	@glyph set status to closed

In changeset 07910d7

#!CommitTicketReference repository="" revision="07910d71c9fc1abd711c1d13924d4dc07ccfd3cd"
Merge pull request #1231 from ChaoticMind/9777-chaoticmind-orphaned-inotify-fd

Author: ChaoticMind

Reviewer: glyph

Fixes: ticket:9777

Don't orphan INotify file descriptors.

[exarkun]

There is some question over whether this change was sensible or not - 073da70#commitcomment-133586418

[exarkun]

I don't know why this change was made. If you want the INotify to go away, the application should call loseConnection. It shouldn't go away because of events it monitors. I would straight-up revert this change. Perhaps a useful new feature to add would be a watch to observe DELETE_SELF events, which does not appear to be possible currently (before or after this change).

[glyph]

Yep, my bad, that was a bad review. If you can call INotify.startReading before calling .watch, it doesn't make sense to connectionLost it even if all watchpoints are deleted, let alone if just one is. I think a revert is called for.

[ChaoticMind]

Hi, looks like indeed this introduces an issue :(

I looked into this a bit. I found an old script I had written when I made this PR to test various scenarios, but clearly not the one raised in the comment.

I think one thing one could try if running the test script is:

0. Start the script
1. In the script, write register
2. In the script, write rmdir (or in a new terminal, rm -rf /tmp/deleteme)
3. In the script, write status (expected: 0)

With this PR, we don't have a leftover file descriptor linked to inotify, but without this PR (i.e. with that line commented), we do.

For some background, I remember having an issue where I had tons of unclosed file descriptors after a long runtime of a twisted process. It could be that the right thing to do was to reap the file descriptors in some other way (the application calling loseConnection() -- I don't remember why exactly, but pretty sure I considered this first but couldn't make it work, I'll try to reproduce) .

I'll try to put in some more thought into it later in the week and try to remember the real context (it was a long time ago), but in the meantime, a second set of eyes on whether or not the use-case in this ticket is also an issue could be helpful, so please feel free to mess with the script if that's helpful.

That being said, this is definitely introducing a regression in wesleys common use case, so feel free to revert this clearly buggy attempt at a fix o.O. You're right, it doesn't make too much conceptual sense either.

Note: I quick n' dirty edited the script (separate gist) to watch two directories to help with validating wesleys case, and I indeed reproduced their issue.

[ChaoticMind]

Also, it looks like the unit test introduced in this PR is of limited use, we actually want to test that we don't have zombie file descriptors, not that loseConnection() specifically is called (so more of an integration than a unit test perhaps -- not sure if you know a good way to do that without real IO)

And of course we want to add a test for the case raised by wesley :)