Skip to content
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

Remove all output to stdout/stderr #62

Open
nd2s opened this issue Jan 26, 2016 · 11 comments
Open

Remove all output to stdout/stderr #62

nd2s opened this issue Jan 26, 2016 · 11 comments

Comments

@nd2s
Copy link

nd2s commented Jan 26, 2016

Because it's a library.

@hreinhardt
Copy link
Owner

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.

@nd2s
Copy link
Author

nd2s commented Jan 27, 2016

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)

@hreinhardt
Copy link
Owner

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.

@nd2s
Copy link
Author

nd2s commented Jan 27, 2016

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...

@hreinhardt
Copy link
Owner

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.

@nd2s
Copy link
Author

nd2s commented Jan 27, 2016

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!

@hreinhardt
Copy link
Owner

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 ().

@nd2s
Copy link
Author

nd2s commented Jan 28, 2016

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.

@hreinhardt
Copy link
Owner

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.

@indrekj
Copy link

indrekj commented Apr 26, 2016

Has there been any progress with auto-reconnect feature?

@hreinhardt
Copy link
Owner

Not that I'm aware of.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants