Exception when HTTP POST fails

[stiang]

On these lines you pattern match for (only) {:ok, response}:

https://github.com/peburrows/goth/blob/fa32be1f65072258bd48a69c81665e95fd248cec/lib/goth/client.ex#L66

https://github.com/peburrows/goth/blob/fa32be1f65072258bd48a69c81665e95fd248cec/lib/goth/client.ex#L87

I see quite a few exceptions like this after switching networks and/or waking my Mac:

** (MatchError) no match of right hand side value: {:error, %HTTPoison.Error{id: nil, reason: :nxdomain}}
    (goth) lib/goth/client.ex:66: Goth.Client.get_access_token/3
    (goth) lib/goth/token.ex:100: Goth.Token.retrieve_and_store!/2
    ...

Is it a deliberate choice that these kinds of errors should cause exceptions, or have you just not gotten around to handling more error cases gracefully? If it’s the latter, would you be open to a PR?

[peburrows]

This was a deliberate choice, but I'm open to the possibility that it was the wrong choice :)

Personally, my preference in those types of error scenarios is to just let it crash. I'm assuming your proposal would be to just return the {:error, %HTTPoison.Error{}} directly instead?

[stiang]

I’m not sure either, and I am certainly no Elixir wizard, but from a developer perspective I had already handled the {:error, err} case and was surprised that I also had to put a rescue block around it to catch other "runtime" errors.

In my case I have a PubSub handler that receives lots of messages, and the "let it crash" strategy will take down the entire app since the frequent crashes in such a short time span trigger the supervisor thresholds for just giving up. So it seems I have to catch the errors and handle them more gracefully, and/or adjust the thresholds to account for a huge number of crashes in a short time. Or I could just handle {:error, err}, as I already do - that was my thinking :)

Anyway, not a big deal - I just get an iffy feeling every time I have to rescue things like this and wanted to check if it was something you had intended differently. Either way is fine!

[peburrows]

The more I've thought about this, the more I think Goth's behavior should change such that it does not raise an exception, but rather returns an {:error, err} tuple.

When I get a chance to dive into this a bit more, I'll work on cleaning these up.

[peburrows]

@stiang give #40 a shot and see if it addresses your issues.

[stiang]

Thanks @peburrows ! I’ve glanced at the code and it looks like it should solve the issue nicely, but I’m swamped at the moment and haven’t found the time to test it yet. Hope to find some time in the next few days, will report back.

[carlpartridge]

@peburrows seeing this issue as well. any updates or timeline on this? would prefer not to add in a rescue if this is still happening.

thanks for your time

[peburrows]

Sorry for the slow responses here, I've been in wedding planning hell for months, but we just got back from our honeymoon, so back to being productive! 🎉

47 should fix this issue, and I plan on merging it and pushing a new release soon.

[peburrows]

Closing this as it should be addressed by #47.