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.

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

Add android retry #131

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
75 changes: 65 additions & 10 deletions server/android_notification_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ type AndroidNotificationServer struct {
AndroidPushSettings AndroidPushSettings
client *messaging.Client
sendTimeout time.Duration
retryTimeout time.Duration
}

// serviceAccount contains a subset of the fields in service-account.json.
Expand All @@ -54,12 +55,13 @@ type serviceAccount struct {
TokenURI string `json:"token_uri"`
}

func NewAndroidNotificationServer(settings AndroidPushSettings, logger *Logger, metrics *metrics, sendTimeoutSecs int) *AndroidNotificationServer {
func NewAndroidNotificationServer(settings AndroidPushSettings, logger *Logger, metrics *metrics, sendTimeoutSecs int, retryTimeoutSecs int) *AndroidNotificationServer {
return &AndroidNotificationServer{
AndroidPushSettings: settings,
metrics: metrics,
logger: logger,
sendTimeout: time.Duration(sendTimeoutSecs) * time.Second,
retryTimeout: time.Duration(retryTimeoutSecs) * time.Second,
}
}

Expand Down Expand Up @@ -166,16 +168,8 @@ func (me *AndroidNotificationServer) SendNotification(msg *PushNotification) Pus
},
}

ctx, cancel := context.WithTimeout(context.Background(), me.sendTimeout)
defer cancel()

me.logger.Infof("Sending android push notification for device=%v type=%v ackId=%v", me.AndroidPushSettings.Type, msg.Type, msg.AckID)

start := time.Now()
_, err := me.client.Send(ctx, fcmMsg)
if me.metrics != nil {
me.metrics.observerNotificationResponse(PushNotifyAndroid, time.Since(start).Seconds())
}
err := me.SendNotificationWithRetry(fcmMsg)

if err != nil {
errorCode, hasStatusCode := getErrorCode(err)
Expand Down Expand Up @@ -233,6 +227,67 @@ func (me *AndroidNotificationServer) SendNotification(msg *PushNotification) Pus
return NewOkPushResponse()
}

func (me *AndroidNotificationServer) SendNotificationWithRetry(fcmMsg *messaging.Message) error {
var err error
waitTime := time.Second

// Keep a general context to make sure the whole retry
// doesn't take longer than the timeout.
generalContext, cancelGeneralContext := context.WithTimeout(context.Background(), me.sendTimeout)
defer cancelGeneralContext()

for retries := 0; retries < MAX_RETRIES; retries++ {
start := time.Now()

retryContext, cancelRetryContext := context.WithTimeout(generalContext, me.retryTimeout)
defer cancelRetryContext()
_, err = me.client.Send(retryContext, fcmMsg)
if me.metrics != nil {
me.metrics.observerNotificationResponse(PushNotifyApple, time.Since(start).Seconds())
Copy link
Member

Choose a reason for hiding this comment

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

Wrong platform key.

}

if err == nil {
break
}

if !isRetryable(err) {
break
}
Comment on lines +249 to +255
Copy link
Member

Choose a reason for hiding this comment

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

Can we combine these in an OR?


me.logger.Errorf("Failed to send android push did=%v retry=%v error=%v", fcmMsg.Token, retries, err)

if retries == MAX_RETRIES-1 {
me.logger.Errorf("Max retries reached did=%v", fcmMsg.Token)
break
}
Comment on lines +259 to +262
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't the for loop take care of this? Why do we need an extra check? And if we do need it, can we remove the condition from the for loop?


select {
case <-generalContext.Done():
case <-time.After(waitTime):
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to incorporate some of this logic as well: https://github.com/firebase/firebase-admin-go/blob/54b81142d35e1b98cfd8687a9e56712c78a0f4ec/internal/http_client.go#L427-L438. We need to get the value of the Retry-After header and set that to be the minimum wait time to retry. Otherwise we will get a rate limited error again.

}

if generalContext.Err() != nil {
me.logger.Infof("Not retrying because context error did=%v retry=%v error=%v", fcmMsg.Token, retries, generalContext.Err())
err = generalContext.Err()
break
}

waitTime *= 2
}

return err
}

func isRetryable(err error) bool {
// We retry the errors based on https://firebase.google.com/docs/cloud-messaging/http-server-ref
return messaging.IsInternal(err) ||
messaging.IsQuotaExceeded(err)

// messaging.IsUnavailable is retried by the default retry config in
// firebase.google.com/go/[email protected]/internal/http_client.go
// messaging.IsUnavailable(err)
}

func getErrorCode(err error) (string, bool) {
if err == nil {
return "", false
Expand Down
6 changes: 3 additions & 3 deletions server/android_notification_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ func TestAndroidInitialize(t *testing.T) {
// Verify error for no service file
pushSettings := AndroidPushSettings{}
cfg.AndroidPushSettings[0] = pushSettings
require.Error(t, NewAndroidNotificationServer(cfg.AndroidPushSettings[0], logger, nil, cfg.SendTimeoutSec).Initialize())
require.Error(t, NewAndroidNotificationServer(cfg.AndroidPushSettings[0], logger, nil, cfg.SendTimeoutSec, cfg.RetryTimeoutSec).Initialize())

f, err := os.CreateTemp("", "example")
require.NoError(t, err)
Expand All @@ -34,7 +34,7 @@ func TestAndroidInitialize(t *testing.T) {
// Verify error for bad JSON
_, err = f.Write([]byte("badJSON"))
require.NoError(t, err)
require.Error(t, NewAndroidNotificationServer(cfg.AndroidPushSettings[0], logger, nil, cfg.SendTimeoutSec).Initialize())
require.Error(t, NewAndroidNotificationServer(cfg.AndroidPushSettings[0], logger, nil, cfg.SendTimeoutSec, cfg.RetryTimeoutSec).Initialize())

require.NoError(t, f.Truncate(0))
_, err = f.Seek(0, 0)
Expand All @@ -46,7 +46,7 @@ func TestAndroidInitialize(t *testing.T) {
ProjectID: "sample",
}))
require.NoError(t, f.Sync())
require.NoError(t, NewAndroidNotificationServer(cfg.AndroidPushSettings[0], logger, nil, cfg.SendTimeoutSec).Initialize())
require.NoError(t, NewAndroidNotificationServer(cfg.AndroidPushSettings[0], logger, nil, cfg.SendTimeoutSec, cfg.RetryTimeoutSec).Initialize())

require.NoError(t, f.Close())
}
Expand Down
2 changes: 1 addition & 1 deletion server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ func (s *Server) Start() {
}

for _, settings := range s.cfg.AndroidPushSettings {
server := NewAndroidNotificationServer(settings, s.logger, m, s.cfg.SendTimeoutSec)
server := NewAndroidNotificationServer(settings, s.logger, m, s.cfg.SendTimeoutSec, s.cfg.RetryTimeoutSec)
err := server.Initialize()
if err != nil {
s.logger.Errorf("Failed to initialize client: %v", err)
Expand Down
Loading