RetryStrategy can not handle thrown DBConnection errors

[mezz]

Due to the way DBConnection.ConnectionError can be sometimes be thrown instead of returned (#119), RetryStrategy does not handle them. Please see run_with_retrying, which does not try/rescue the call to fun here: https://github.com/lexhide/xandra/blob/2ba4701930ec0c515070f15fd217b870deb57560/lib/xandra/retry_strategy.ex#L126-L127

I ran into this situation:
On two different processes nearly the same error was returned on one and thrown on the other from the same call spot at nearly the same time, which was very unexpected and confusing.
The retry strategy could handle one and I logged it as a warning, but the thrown error just killed the process.

[whatyouhide]

@mezz I think it would make sense to rescue DBConnection.ConnectionErrors since they seem to be raised. Would you like to submit a PR? I don't have cycles currently to work on Xandra :)

[mezz]

Thanks @whatyouhide, I have just created a PR for this here: #184

[qcam]

On two different processes nearly the same error was returned on one and thrown on the other from the same call spot at nearly the same time, which was very unexpected and confusing.

I think I'm more inclined towards this being a DBConnection issue rather than a Xandra issue. 🤔

[mezz]

@qcam I am not entirely sure this is an inconsistency in DBConnection, it seems to be in how Xandra handles calling it in different situations.
I found this previous issue, where it seems like Xandra previously chose to handle this case inconsistently: #119

[qcam]

I don't think that's Xandra handling the return differently. Xandra does handle when the call returns with {:error, error} (as what the DBConnection docs says), but rather DBConnection would raise when it fails to check out a connection. The raising behavior (what it raises on, what it raises with) is not really documented on DBConnection, which means technically that could change any time. I personally don't see this as a bug on Xandra :).

I guess the question becomes if we would want to retry on checkout failure. IMO if we do, we might want to guard against this behavior explicitly, rather than rescuing every DBConnection.ConnectionError exception.

[mezz]

Thanks @qcam, I think you have better context on this issue than I do.

I don't think it makes sense to raise some errors and return others. They should be handled one documented way and differentiated by the type of error instead.

If this DBConnection behavior can be changed without breaking things significantly, I agree it would be best to fix it there.

If in general DBConnection leaves errors and retries up to the caller, I think that by consistently returning the error with {:error, error} it would allow consumers like Xandra to handle the error however they want, either by retrying or failing.

[whatyouhide]

@qcam raises good points. @mezz it would be great if you could report this issue to DBConnection? In the meantime, I'll close this issue and #184.