-
Notifications
You must be signed in to change notification settings - Fork 83
spaceship ITC client hardening (#150). #235
Conversation
raise ex # re-raise the exception | ||
end | ||
|
||
private | ||
|
||
# this mostly for troubleshooting while harderning the client | ||
def debug_count_recovered | ||
@recoveriesCount = (@recoveriesCount || 0 ) + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space inside parentheses detected.
Use snake_case for variable names.
5ffb76f
to
a61c1d0
Compare
return block.call | ||
rescue Spaceship::TunesClient::ITunesConnectTemporaryError => ex | ||
unless (tries -= 1).zero? | ||
msg = "ITC temporary save error received: '#{ex.message}'. Retrying after 60 seconds (remaining: #{tries})..." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have access to the tries
variable here? If yes, we could start by waiting just like a few seconds, and then go up to 1-2 minutes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we could. Right now I am aiming for resilience, then fine tuning. In the end I would love to move some of this into either a faraday middleware, or using a custom more declarative "retrier".
For example, out of the last 5 runs, they all ended up failing here. I increased it to 60 seconds and it still fails sometimes.
Here are the last 2, with a 60 second sleep in between retries:
ITC temporary save error received: 'We're temporarily unable to save your changes. Please try again later.'. Retrying after 60 seconds (remaining: 1)...
ESC[37m[19:47:24]: ESC[0mFailed / completed after 5 hour(s) 16 min(s) 32 sec(s)
/Users/lacostej/.rvm/gems/ruby-2.2.4/gems/spaceship-0.19.3/lib/spaceship/tunes/tunes_client.rb:218:in `handle_itc_response': We're temporarily unable to save your changes. Please try again later. (Spaceship::TunesClient::ITunesConnectTemporaryError)
from /Users/lacostej/.rvm/gems/ruby-2.2.4/gems/spaceship-0.19.3/lib/spaceship/tunes/tunes_client.rb:361:in `block in update_app_version!'
ITC temporary save error received: 'We're temporarily unable to save your changes. Please try again later.'. Retrying after 60 seconds (remaining: 1)...
ESC[37m[21:20:34]: ESC[0mFailed / completed after 1 hour(s) 33 min(s) 8 sec(s)
/Users/lacostej/.rvm/gems/ruby-2.2.4/gems/spaceship-0.19.3/lib/spaceship/tunes/tunes_client.rb:218:in `handle_itc_response': We're temporarily unable to save your changes. Please try again later. (Spaceship::TunesClient::ITunesConnectTemporaryError)
from /Users/lacostej/.rvm/gems/ruby-2.2.4/gems/spaceship-0.19.3/lib/spaceship/tunes/tunes_client.rb:361:in `block in update_app_version!'
from /Users/lacostej/.rvm/gems/ruby-2.2.4/gems/spaceship-0.19.3/lib/spaceship/tunes/tunes_client.rb:795:in `call'
In the one that lasted 5 h 30 min, I had
- 0 timeout / connection failed (they appear more seldomly now given the 300 seconds
- 20+ auth lost
- 6 unable to save (last one fatal)
connection timeout)
This looks really promising, great job. Pinging @snatchev, as he's also a |
a61c1d0
to
a579667
Compare
|
||
describe Spaceship::Client do | ||
class TestClient < Spaceship::Client | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra empty line detected at class body beginning.
6823514
to
df4b3b8
Compare
|
||
ideas | ||
* parametrize default timeout ? | ||
* make default timeout client specific ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before merging this needs to be moved out into separate GitHub issues. I can add a stability
tag for you 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the whole file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I didn't intend to submit it as it.
This looks great, thanks again for this PR, I added some comments 👍 |
msg = "ITC temporary save error received: '#{ex.message}'. Retrying after 60 seconds (remaining: #{tries})..." | ||
puts msg | ||
logger.warn msg | ||
sleep 60 unless defined?SpecHelper # unless FastlaneCore::Helper.is_test? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space after keyword defined?
is missing.
@lacostej looks good to me. |
I just did the final review, this looks perfect from my side. Let's squash the commits and I'll ship a new version 🚀 |
4215ff3
to
0010193
Compare
0010193
to
cf588f5
Compare
* handles better ITC timeouts (by extending the default timeout to 5 min) * handles HTTP/SSL failures (by retrying them) those are probably openssl&client dependent * handles ITC 401 issues (by login again) * handles better temporary ITC issue to save ('try again later') (by ... retrying again) Also * cache some of the data to avoid querying the server too much Assisted by Krzysztof Daniszewski <krzysztof@daniszewski.it>
* handles better ITC timeouts (by extending the default timeout to 5 min) * handles HTTP/SSL failures (by retrying them) those are probably openssl&client dependent * handles ITC 401 issues (by login again) * handles better temporary ITC issue to save ('try again later') (by ... retrying again) Also * cache some of the data to avoid querying the server too much Assisted by Krzysztof Daniszewski <krzysztof@daniszewski.it>
cf588f5
to
1104727
Compare
spaceship ITC client hardening (#150).
Thanks again for this 👍 |
The following set of patches allow to make ITC communication more resilient.
It:
Several things are to be discussed, in particular, how to implement re-login in the case of spaceship client#login being used with 2 parameters (password is currently not remembered in that case)
Last run:
It is resilient but slow.
Updated with unit tests.
Note that the issue mentioned here, is tentatively covered by df4b3b8. This is one solution, but I am unsure we want to solve it that way.