New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ghc 7.6 compability patch doesn't seem correct #3
Comments
Hi, with the current (not optimal) design, we do actually want to catch all errors. 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. 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. |
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 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 Thanks! |
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. 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. |
Thanks for the async/IOError explanation. I honestly confused non-IOErrors and async errors; the 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. |
Hi,
I've looked at commit 5f96f08 which fixed compatibility with ghc 7.6, but the following worries me:
Before, the code was using
Prelude.catch
, which only catches IO errors. This new variant catches all errors, including things likeUserInterrupts
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.The text was updated successfully, but these errors were encountered: