Add support for catching IN_Q_OVERFLOW to inotify

[Gernot Vormayr]

The IN_Q_OVERFLOW event has a -1 as wd and so no callback gets ever called if it happens.

This adds a callback special callback to INotify for this case.

I'm not very happy with the testing implementation (it tries to find out event max and then tries to generate more events than fit into the queue), but I can't think of a better way right now

[Gernot Vormayr]

Patch to add support for overflow callback

[Jean-Paul Calderone]

(In [41009]) Branching to 'inotify-overflow-6722'

[Jean-Paul Calderone]

(In [41010]) apply inotify-overflow.patch

refs #6722

[Jean-Paul Calderone]

Thanks! Sorry about the long delay in getting this patch reviewed.

1. in test_overflowEvent
   1. Regarding the concerns about the strategy used in the tests, I think that it will be very difficult to avoid these operations and still provide good test coverage. If the number of unit tests for the overflow callback grows beyond one then I think it *will* make sense to isolate the system interactions involved in the single test added by this patch. Ideally there should be a minimum number of this kind of "system integration" or "functional" test. Other tests should be built up using a verified fake. This way we have the confidence that the functionality really works with real systems (because we have a functional test that demonstrates this) but we also still have a fast test suite (because tests for everything *apart* from "does this drive the system interface correctly" don't run against the real system, they run against a fake that has been demonstrated to have the same interface as the real system but which can be purely in-memory, more easily customizable (eg can have a max event queue size of 3 instead of 2 14), and is almost certainly faster, more predictable, and easier to debug when problems arise.
   2. You want to avoid writing conditionals on Deferred.called in almost all circumstances. This code should use another signal such as an explicit alreadyOverflowed flag.
   3. Since the Python 3 porting work was started, code written with FilePath should use explicit byte strings (since unicode paths are not yet properly supported by FilePath).
   4. FilePath.getContent will help shorten the code for reading the max queued events value.
   5. "Bare" except statements should be avoided. Perhaps this will end up handling a NameError or AttributeError caused by a programming mistake. Just handle the exceptions you're actually anticipating so that surprises are visible instead of hidden.
   6. Additionally, I'm not sure it makes sense to provide a default value here. If the system configured max event queue size cannot be determined then it probably makes more sense to skip the test.
   7. The test is quite slow. :( Presumably that goes with creating 2 14 new files. Perhaps using the IN_OPEN and IN_CLOSE_NOWRITE events and then just opening and closing a file a lot of times would exercise the implementation just as well but run in less time. (A quick experiment suggests this is about 40% faster - which is a nice improvement but the test is still noticably slow. :/)
2. in twisted/internet/inotify.py
   1. overflow should be documented in __init__'s docstring (the class docstring can just refer readers to the __init__ docstring)
   2. INotify.watch accepts a list of callbacks instead of just a single callback. I'm not sure exactly why this is so. If it is a good idea for watch callbacks, though, perhaps it is also a good idea for the overflow callback. Because the motivation here is essentially just "cargo cult" I am more than happy to hear counter-arguments.
   3. C{f} isn't meaningful. :) How about something like The application-supplied object which is called to deliver overflow notification.?
   4. Perhaps it would be correct to continue after calling self._overflow() in _doRead? I think that an overflow event cannot carry information for any other watch (the inotify man page says wd will be -1 in this case; according to the inotify_add_watch man page, -1 can never be a valid watch descriptor (it indicates an error in the inotify_add_watch call)).
3. ​Buildbot points out some other minor issues (only some of which are totally bogus, sorry about those - ignore anything that's obviously not related to this change)
4. The change should be accompanied by a news fragment as well.

Thanks again for working on this. Since a lot of these points are quite simple and mechanical I think I'll go ahead and check some improvements into the branch. Perhaps I'll get it into sufficiently good shape that it'll worth getting another review.

[Jean-Paul Calderone]

You want to avoid writing conditionals on Deferred.called in almost all circumstances. This code should use another signal such as an explicit alreadyOverflowed flag.

r41011

Since the Python 3 porting work was started, code written with FilePath should use explicit byte strings (since unicode paths are not yet properly supported by FilePath).

r41012

FilePath.getContent will help shorten the code for reading the max queued events value.

r41013

"Bare" except statements should be avoided. Perhaps this will end up handling a NameError or AttributeError caused by a programming mistake. Just handle the exceptions you're actually anticipating so that surprises are visible instead of hidden.

Since I'm not actually sure what exceptions this was meant to handle, I just removed the exception handling. r41014

Additionally, I'm not sure it makes sense to provide a default value here. If the system configured max event queue size cannot be determined then it probably makes more sense to skip the test.

If I figure out what exceptions might happen loading this value then I'll come back and add some skip logic.

The test is quite slow. :( Presumably that goes with creating 2 14 new files. Perhaps using the IN_OPEN and IN_CLOSE_NOWRITE events and then just opening and closing a file a lot of times would exercise the implementation just as well but run in less time. (A quick experiment suggests this is about 40% faster - which is a nice improvement but the test is still noticably slow. :/)

r41015

overflow should be documented in __init__'s docstring (the class docstring can just refer readers to the __init__ docstring)

r41016

INotify.watch accepts a list of callbacks instead of just a single callback. I'm not sure exactly why this is so. If it is a good idea for watch callbacks, though, perhaps it is also a good idea for the overflow callback. Because the motivation here is essentially just "cargo cult" I am more than happy to hear counter-arguments.

I still want some more input from other people on this.

C{f} isn't meaningful. :) How about something like The application-supplied object which is called to deliver overflow notification.?

Also addressed by r41016.

Perhaps it would be correct to continue after calling self._overflow() in _doRead? I think that an overflow event cannot carry information for any other watch (the inotify man page says wd will be -1 in this case; according to the inotify_add_watch man page, -1 can never be a valid watch descriptor (it indicates an error in the inotify_add_watch call)).

r41017

Buildbot points out some other minor issues (only some of which are totally bogus, sorry about those - ignore anything that's obviously not related to this change)

r41018

The change should be accompanied by a news fragment as well.

r41019

Maybe that's everything. I did skip addressing some of the review comments though. Any additional thoughts on those appreciated. New build triggered with the latest code, too.

[Jean-Paul Calderone]

A few follow-up revisions to discover and fix a bug I introduced in r41018. The subdir.remove() generated one final event which was necessary to actually tip the queue into overflow mode. Accounted for that in r41021 (after a first failed attempt in r41020) by adding one more iteration to the loop generating events. Build updated yet again.

[Gernot Vormayr]

Fake events in overflow test

[Gernot Vormayr]

Thanks for cleaning it up a bit.

@1.1 You are right this is maybe a bad way to test this. I initially thought it would be harder to fake the inotify interface. Turns out it is quite simple. attachment:"inotify-overflow-fake-test.patch"​ (should apply to r41021) changes the test to use a FakeINotify class that can fake events.

@2.2 I'm sorry I have no idea either and I don't know which way would be better and am totally fine with changing it to callbacks.

@2.1 & 2.3 Sorry I had no idea how the documentation works and just copy pasted it together from the rest of the inotify documentation at the time.

@2.4 Yeah continue is a very good idea. I overlooked that one.

Thanks for fixing the other things and sorry for my late answer - was away on holiday.

[Gernot Vormayr]

After a discussion on IRC I think my patch is not adequate enough.

Possible ways to move forward:

• create a verified fake that passes the unit tests
• use the original proposed way to test

Since creating the verified fake will be a bigger amount of work, I put this one on hold, till I have fleshed out something, which will be independet from this patch and open up another ticket, or if this project is not worth it, I'll clean up this patch.