Remove all output to stdout/stderr
Opened this issue · 11 comments
Because it's a library.
I think we mostly print stuff to stderr in case of developer mistakes. Just silently ignoring these errors doesn't seem like a good idea to me.
I think in case the developer uses the library wrong ("breach of contract") it should throw an exception so one can't ignore it. Although printing wouldn't be that problematic in this case because one can just fix the problem to silence the library.
But my problem here is that it seems to print to stderr additionally to throwing an exception when connecting to the broker fails (openConnection): Error connecting to ("localhost",5672): connect: does not exist (Connection refused)
Throwing an exception brings up the problem of where to throw the exception, since it may happen asynchronously.
Regarding the specific "Error connecting" message, we could probably just remove it, though the information may be useful if somebody uses multiple brokers.
How about adding the reason of failure to each broker tuple in the the exception message?
Could not connect to any of the provided brokers: [("localhost",5672, "Connection refused")]
Or maybe a debug setting would make sense...
We only throw an exception if all the brokers failed, but in that case it's pretty clear what went wrong.
I was thinking more about the situation when one of your brokers doesn't work. It might make sense to offer an API that tells the user which of the brokers were unavailabe. But I'm not sure how exactly that API should behave.
If it's OK for you I will just remove the "Error connecting..." message and we can figure out an API for unavailabe brokers if the need arises in the future.
I know, but printing is mostly useless anyway because it won't reach the user in a lot of setups (for example if you do logging via syslog + use daemonize that closes stdout/stderr), or you need to log in a specific output format. That's why I suggested the exception thing -> that's the only scenario where the program can actually work with that information and react appropriately.
I'm just concerned about the stdterr output that messes up my program, so yes removal would be great. Thanks!
I have pushed 0.13.1 to Hackage with this single change.
An alternative to the exceptions approach might be to allow the user to setup a specific handler for errors, something like setErrorLogger :: Connection -> (String -> IO ()) -> IO ().
Works nicely, thank you very much.
Not sure what's the right approach. I hate exceptions, but loads of handlers could get messy as well.
Maybe it would also make sense to add a higher level system that automatically handles reconnects and offers functions for adding logging and queue/exchange/... setup handlers (and catches all AMQP-specific exceptions). That's kind of what I was implementing (not finished yet and will take a while as I don't have a lot of time atm). I guess all people implementing a production-ready system will need this.
Having some kind of higher-level recovery system sounds interesting. The Ruby AMQP library seems to offer something similar.
Though I'm kind of skeptical whether such a system could be generic enough that it would fit the majority of use-cases.
If you come up with something that seems good, we can talk about merging it into the library.
Has there been any progress with auto-reconnect feature?
Not that I'm aware of.