Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatic retry with exponential backoff #58

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
+85 −36
Open

Automatic retry with exponential backoff #58

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9fdc66a
Retry with Exponential backoff
Alan-M-Thomaz Mar 28, 2019
ad2e977
Update to FCM URLs for device groups features
Alan-M-Thomaz Mar 28, 2019
f99a4c3
fix tests to use FCM device group URL
Alan-M-Thomaz Mar 29, 2019
3607446
fix,refactor and added tests for retry
Alan-M-Thomaz Mar 29, 2019
46079de
cover all retryable errors
Alan-M-Thomaz Mar 29, 2019
f86baf6
removing Faraday raise_error middleware.
Alan-M-Thomaz Apr 1, 2019
a8742e0
fix syntax in specs for ruby version 2.1 or lower
Alan-M-Thomaz Apr 1, 2019
8620262
refactor notification_key related methods
Alan-M-Thomaz Apr 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 26 additions & 11 deletions lib/fcm.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ class FCM
FORMAT = :json

# constants
GROUP_NOTIFICATION_BASE_URI = 'https://android.googleapis.com'
INSTANCE_ID_API = 'https://iid.googleapis.com'
TOPIC_REGEX = /[a-zA-Z0-9\-_.~%]+/

Expand All @@ -34,6 +33,7 @@ def initialize(api_key, client_options = {})
def send_notification(registration_ids, options = {})
post_body = build_post_body(registration_ids, options)


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👇here why not just using execute_notifications(post_body) instead

for_uri(BASE_URI) do |connection|
response = connection.post('/fcm/send', post_body.to_json)
build_response(response, registration_ids)
Expand All @@ -49,8 +49,8 @@ def create_notification_key(key_name, project_id, registration_ids = [])
'project_id' => project_id
}

for_uri(GROUP_NOTIFICATION_BASE_URI, extra_headers) do |connection|
response = connection.post('/gcm/notification', post_body.to_json)
for_uri(BASE_URI, extra_headers) do |connection|
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Alan-M-Thomaz

since there are four places where we need to call /fcm/notification

  • create_notification_key(post)
  • add_registration_ids(post)
  • remove_registration_ids(post)
  • recover_notification_key(get)

maybe it would be better if we refactor this api call into a separate private method, like what execute_notification method does..

def update_group_messaging_setting(body)
  for_uri(BASE_URI, extra_headers) do |connection|
      response = connection.post('/fcm/notification', body.to_json)
      build_response(response)
  end
end

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, except for recover_notification_key, because it's a get request

response = connection.post('/fcm/notification', post_body.to_json)
build_response(response)
end
end
Expand All @@ -65,8 +65,8 @@ def add_registration_ids(key_name, project_id, notification_key, registration_id
'project_id' => project_id
}

for_uri(GROUP_NOTIFICATION_BASE_URI, extra_headers) do |connection|
response = connection.post('/gcm/notification', post_body.to_json)
for_uri(BASE_URI, extra_headers) do |connection|
response = connection.post('/fcm/notification', post_body.to_json)
build_response(response)
end
end
Expand All @@ -81,8 +81,8 @@ def remove_registration_ids(key_name, project_id, notification_key, registration
'project_id' => project_id
}

for_uri(GROUP_NOTIFICATION_BASE_URI, extra_headers) do |connection|
response = connection.post('/gcm/notification', post_body.to_json)
for_uri(BASE_URI, extra_headers) do |connection|
response = connection.post('/fcm/notification', post_body.to_json)
build_response(response)
end
end
Expand All @@ -94,13 +94,13 @@ def recover_notification_key(key_name, project_id)
notification_key_name: key_name
}
}

extra_headers = {
'project_id' => project_id
}

for_uri(GROUP_NOTIFICATION_BASE_URI, extra_headers) do |connection|
response = connection.get('/gcm/notification', params)
for_uri(BASE_URI, extra_headers) do |connection|
response = connection.get('/fcm/notification', params)
build_response(response)
end
end
Expand Down Expand Up @@ -144,7 +144,7 @@ def get_instance_id_info iid_token, options={}
params = {
query: options
}

for_uri(INSTANCE_ID_API) do |connection|
response = connection.get('/iid/info/'+iid_token, params)
build_response(response)
Expand Down Expand Up @@ -177,7 +177,22 @@ def send_to_topic_condition(condition, options = {})
private

def for_uri(uri, extra_headers = {})
retry_if_func = lambda do |env, exception|
retryable_errors = [
"Unavailable", "InternalServerError",
"DeviceMessageRateExceeded", "TopicsMessageRateExceeded"]
if defined?(exception.response) && defined?(exception.response.status) && exception.response.status == 200
body = JSON.parse(exception.response.body)
body["results"] != nil && body["results"].any? { |result| retryable_errors.include? result["error"]}
else
true
end
end
retryable_exceptions = Faraday::Request::Retry::DEFAULT_EXCEPTIONS
connection = ::Faraday.new(:url => uri) do |faraday|
faraday.request :retry, max: 5, interval: 0.1, interval_randomness: 0.5, backoff_factor: 2,
exceptions: retryable_exceptions, retry_statuses: [200, *(500..599)], methods: [],
retry_if: retry_if_func
faraday.adapter Faraday.default_adapter
faraday.headers["Content-Type"] = "application/json"
faraday.headers["Authorization"] = "key=#{api_key}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before

Authorization: key=AIzaSyZ-1u...0GBYzPu7Udno5aA
After

Authorization: Bearer ya29.ElqKBGN2Ri_Uz...HnS_uNreA

https://firebase.google.com/docs/cloud-messaging/migrate-v1

Expand Down
58 changes: 51 additions & 7 deletions spec/fcm_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

describe FCM do
let(:send_url) { "#{FCM::BASE_URI}/fcm/send" }
let(:group_notification_base_uri) { "#{FCM::GROUP_NOTIFICATION_BASE_URI}/gcm/notification" }
let(:group_notification_base_uri) { "#{FCM::BASE_URI}/fcm/notification" }
let(:api_key) { 'AIzaSyB-1uEai2WiUapxCs2Q0GZYzPu7Udno5aA' }
let(:registration_id) { '42' }
let(:registration_ids) { ['42'] }
Expand Down Expand Up @@ -90,7 +90,7 @@
stub_request(:post, send_url)
.with(body: '{"registration_ids":["42"],"data":{"score":"5x1","time":"15:10"}}',
headers: valid_request_headers)
.to_return(status: 200, body: '', headers: {})
.to_return(status: 200, body: '{}', headers: {})
end
before do
end
Expand All @@ -106,13 +106,13 @@
stub_request(:post, send_url)
.with(body: '{"to":"/topics/TopicA","data":{"score":"5x1","time":"15:10"}}',
headers: valid_request_headers)
.to_return(status: 200, body: '', headers: {})
.to_return(status: 200, body: '{}', headers: {})
end
let!(:stub_with_invalid_topic) do
stub_request(:post, send_url)
.with(body: '{"condition":"/topics/TopicA$","data":{"score":"5x1","time":"15:10"}}',
headers: valid_request_headers)
.to_return(status: 200, body: '', headers: {})
.to_return(status: 200, body: '{}', headers: {})
end

describe "#send_to_topic" do
Expand All @@ -135,19 +135,19 @@
stub_request(:post, send_url)
.with(body: '{"condition":"\'TopicA\' in topics && (\'TopicB\' in topics || \'TopicC\' in topics)","data":{"score":"5x1","time":"15:10"}}',
headers: valid_request_headers)
.to_return(status: 200, body: '', headers: {})
.to_return(status: 200, body: '{}', headers: {})
end
let!(:stub_with_invalid_condition) do
stub_request(:post, send_url)
.with(body: '{"condition":"\'TopicA\' in topics and some other text (\'TopicB\' in topics || \'TopicC\' in topics)","data":{"score":"5x1","time":"15:10"}}',
headers: valid_request_headers)
.to_return(status: 200, body: '', headers: {})
.to_return(status: 200, body: '{}', headers: {})
end
let!(:stub_with_invalid_condition_topic) do
stub_request(:post, send_url)
.with(body: '{"condition":"\'TopicA$\' in topics","data":{"score":"5x1","time":"15:10"}}',
headers: valid_request_headers)
.to_return(status: 200, body: '', headers: {})
.to_return(status: 200, body: '{}', headers: {})
end

describe "#send_to_topic_condition" do
Expand Down Expand Up @@ -338,6 +338,50 @@
)
end
end

context 'when send_notification responds with 5XX or 200+error:unavailable' do
subject { FCM.new(api_key) }

let(:valid_response_body) do
{
failure: 0, canonical_ids: 0, results: [{ message_id: '0:1385025861956342%572c22801bb3' }]
}
end

let(:error_response_body) do
{
failure: 1, canonical_ids: 0, results: [{ error: 'Unavailable' }]
}
end


let(:stub_fcm_send_request_unavaible_server) do
i = 0
stub_request(:post, send_url).with(
body: "{\"registration_ids\":[\"42\"]}",
headers: {
'Accept'=>'*/*',
'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3',
'User-Agent'=>'Faraday v0.15.4'
}).to_return do |stub|
body = {}
status = ( i == 0) ? 501 : 200
body = error_response_body if i == 1
body = valid_response_body if i == 0
i += 1
{status: status, headers: { :"Retry-After" => "1"}, body: body.to_json}
end
end

before(:each) do
stub_fcm_send_request_unavaible_server
end

it 'should retry 2 times' do
subject.send( registration_ids)
stub_fcm_send_request_unavaible_server.should have_been_made.times(3)
end
end
end

describe 'sending group notifications' do
Expand Down