manageplaces / Stompex

A STOMP client for Elixir
MIT License
12 stars 7 forks source link

Add exception handling. #11

Open shymega opened 5 years ago

shymega commented 5 years ago

Stompex should handle exceptions gracefully, whilst alerting the operator of the Elixir node that an error did occur when handling STOMP frames, but not aborting operations completely.

pareeohnos commented 5 years ago

Think this needs some thought. The general attitude in Elixir is "let it fail", but handling the errors is kinda going against this. That said, some errors would likely be ones that shouldn't result in a completely failure. I think it should certainly handle some errors, but to a point. If for example the connection fails, it should handle this and automatically attempt to reconnect. If it repeatedly fails to do this then it should eventually give up.

Other kinds of errors though I agree, they should be handled and returned - we just need to figure out which ones and how they should be handled. it could also be a configuration option.

shymega commented 5 years ago

You raise good points here, @pareeohnos.

I think your reply is spot-on, I did miss some of these issues. The errors I would personally handle gracefully are connection errors/timeouts and malformed STOMP frames.

I think, these are the main types of errors to be handled. Thoughts?

pareeohnos commented 5 years ago

Yeah I think that covers it. Though I'd say gracefully to a point. If for instance the connection drops out, and stompex keeps trying to reconnect but it keeps failing, then it should raise an actual exception

shymega commented 5 years ago

Agreed. We should add a configuration option to either: immediately reconnect, or do an exponential backoff before reconnecting. I use an exponential backoff for my use of Stompex with National/Network Rail to prevent being banned by their firewall and reduce strain on the brokers. Having this integrated with Stompex would certainly reduce the amount of code I have to handle reconnection logic.

pareeohnos commented 5 years ago

I'd go with just an exponential backoff, but with the first reconnect attempt being immediate. Though I THINK connect is already providing this backoff logic in Stompex?

shymega commented 5 years ago

Sounds good to me. Looking at the code for connect(), it does specify a socket timeout (upon connec ting? 🤔) for the initial connection. But it doesn't do any backoff logic..

pareeohnos commented 5 years ago

Connection does support it, think it just wasn't used. But I'd rather drop Connection entirely and just implement the logic as needed

shymega commented 5 years ago

I'd rather drop it [Connection] as well, which relates to #12... which will - hopefully - leave us with no deps for the :prod environment!

pareeohnos commented 5 years ago

Yeah think that's the only dependency so would make it clean

shymega commented 5 years ago

I have a few dependencies for the :dev and :test branch locally here - credo for CI linting, and ex_doc for documentation generation.. but I'm testing them at the moment, and even then, they wouldn't be included in production..

pareeohnos commented 5 years ago

nah that's fine, always gonna have a few more dev dependencies

shymega commented 5 years ago

Cool, sounds good to me.

We might not actually need ex_doc, as hexdocs.pm generates documentation when we push a new Hex package, added bonus! 👌

pareeohnos commented 5 years ago

hmm you sure they do? I was looking through their source recently, and it doesn't seem to do anything more than publish? If they were to build, they have to be able to compile libraries which may potentially have missing config or files required to compile. Not sure though

shymega commented 5 years ago

Looks like they do, from here.

Documentation will be built and published automatically. To publish a package without documentation run mix hex.publish package or to only publish documentation run mix hex.publish docs.

I don't think they build the package per-se, only generate documentation...

pareeohnos commented 5 years ago

I think that's when you run mix hex.publish - I think it builds it on your machine, and publishes the result.

shymega commented 5 years ago

Oh, right, fair enough.

I'll keep ex_docs in the :dev environment.