ghc 7.6 compability patch doesn't seem correct

[iustin]

Hi,

I've looked at commit 5f96f0887e9b93d8f8d4e37060451ba66fe0d9a1 which fixed compatibility with ghc 7.6, but the following worries me:

ignore_failure :: SomeException -> IO ()
ignore_failure _ = return ()

Before, the code was using Prelude.catch, which only catches IO errors. This new variant catches all errors, including things like UserInterrupts and similar. Per the section "Catching all exceptions" in Control.Exception documentation, this is not recommended.

So I believe the signature should be changed to ignore_failure :: IOError -> IO(). Let me know if you want a pull request.

[kolmodin]

Hi,

with the current (not optimal) design, we do actually want to catch all errors. The handler we're catching errors from is supplied by the user of the library, and we don't know what it'll do. If it would throw an exception (IO or otherwise) it'd silently kill the dispatcher and you would simply not receive any further events. By catching all the errors we make sure this does not happen, even if a handler throws an exception it does not stop further events from being processed.

The downside is obviously that the exceptions are silently dropped and that can make your debugging a lot harder. An alternative approach for library would be something like

inotify :: IO (INotify, Chan Event)
addInotify :: INotify -> [EventVariety] -> FilePath -> IO WatchDescriptor

where you get a channel of events, and you can process them yourself and handling any exceptions yourself.

A problem with that approach is that if you don't read the Chan and process the events quickly enough you can get a lot of events in the Chan. Yet a different approach would be to let the users of the library also do the reading of events.

inotify :: IO INotify
addInotify :: INotify -> [EventVariety] -> FilePath -> IO WatchDescriptor
readEvents :: INotify -> IO [Event]

Here there are no threads, and no user code to throw exceptions. I think this option is the most robust as we'd let the Linux kernel do the overflow handling.

[iustin]

Hmm, I'll have to think on your two proposed approaches…

Note that the only thing that worries me is not the dropping of all I/O exceptions, but the fact that even a ThreadKilled exception is being ignored. For example, if user code wants to shutdown the inotify system via killINotify, and this call is executed during the processing of an event, it will have no effect, leaving the dispatcher or the event reader threads alive.

If the code would do what it did before, just drop I/O exceptions, that would be OK until a better API is in place.

After reading the code a bit, it seems that the second of your proposal makes sense; users will have to manage an extra thread, but the API is cleaner indeed. In the first approach, you are worried about overflow/lots of mesages in the Chan, since a background thread (start_thread currently) reads from kernel and inserts the messages in the channel, right? If so, then yes, the second API looks even better.

Thanks!

[kolmodin]

Nice catch about ThreadKilled! I've sent a patch to mask asynchronous exceptions while the user's code runs, they'll be raised once we're back in the handler. Any other kind of exception will be ignored. The risk of dropping only IO exceptions is that the user's code can raise a wide range of other exceptions (like DivideByZero) which then would kill the handler thread. If we drop any exception except the asynchronous we know that the thread will keep going.

It's understood that this approach is flawed and the current state might not be perfect, but should be a lot better than what we just had.

With the second of the proposals above, the users can choose if they want an extra thread or however they want to solve it. For performance they can also wait to call the readEvents function until there is a certain number of bytes in the handle (or a timeout occurs), it'd improve performance. The current implementation reads as soon as it finished the last read, which is not efficient. And yes, my worries with the first proposal is mainly that we can get a lot of unhandled messages.

[iustin]

Thanks for the async/IOError explanation. I honestly confused non-IOErrors and async errors; the mask_ change is good (as good as it can be made in the context of async exceptions).

Looking forward to the new API then, in the meantime this should be good. Feel free to rename this bug or close it an open a separate one for the API change.

[kolmodin]

Ok, thanks for reporting the issue. Hopefully this will be the last fix needed in the 0.3.x branch before the new API in 0.4.

I consider this bug closed with commit a9c897c2.

Opening issue #4 to track the new API.