search

RubyMoney / google_currency

Ruby Money::Bank interface for the Google Currency exchange data
http://rubymoney.github.com/google_currency
MIT License
182 stars 90 forks source link

Retry common intermittent network exceptions #50

Closed doofdoofsf closed 8 years ago

doofdoofsf commented 8 years ago

https://www.google.com/finance/converter, has been quite flaky of late, commonly returning intermittent exceptions: [Errno::ECONNREFUSED, OpenURI::HTTPError, Errno::ENETUNREACH]

This has been causing some pain for us.

This PR implements a simply retry. We've had this code running in production for over a month and seems to solve the problem completely. So far at least. It would be great to get this merged in if you like the direction.

antstorm commented 8 years ago

@doofdoofsf great job! Couple of things before we can merge this in:

  1. It definitely needs some tests for the new stuff
  2. It should be possible to configure amount of retries. It now defaults to 3, which may be undesirable for some people, especially with a progressive delay.
  3. I don't think catching OpenURI::HTTPError by default is a good idea, since it might mean that the currency or exchange rate is not available (no retries would solve this).
doofdoofsf commented 8 years ago

Anthony, thanks for the quick response.

It definitely needs some tests for the new stuff

Shame on me. I'll write some.

It should be possible to configure amount of retries. It now defaults to 3, which may be undesirable for some people, especially with a progressive delay.

Agreed. I'll do that.

don't think catching OpenURI::HTTPError by default is a good idea, since it might mean that the currency or exchange rate is not available (no retries would solve this).

I don't recall the category of problem that was generating this exception. I'll try taking it out and revisiting it. Perhaps we should make retry_exceptions a configuration option too, defaulting to [Errno::ECONNREFUSED, Errno::ENETUNREACH]?

antstorm commented 8 years ago

I don't recall the category of problem that was generating this exception. I'll try taking it out and revisiting it. Perhaps we should make retry_exceptions a configuration option too, defaulting to [Errno::ECONNREFUSED, Errno::ENETUNREACH]?

Yeah, that should be great!

antstorm commented 8 years ago

@doofdoofsf have you given up on this one? :)

doofdoofsf commented 8 years ago

@antstorm Well, kinda. I've been making much more extensive changes to the gem to increase reliability. Not clear to me if I should PR or just maintain our own private fork.

antstorm commented 8 years ago

@doofdoofsf I'm more than happy to merge these in, I'm sure other devs would benefit from these changes as well

doofdoofsf commented 8 years ago

@antstorm thanks. let me see if i can grab some time to wrap things up and improve the test coverage etc. i agree others would benefit. our requests have been 100% reliable since the latest round of changes, which is nice.

antstorm commented 8 years ago

@doofdoofsf sounds great! Ping me if you need any help with writing those tests and/or getting them to pass