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

chore: fix race condition with push iOS notification #533

Merged
merged 1 commit into from
Jul 14, 2020
Merged
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
17 changes: 6 additions & 11 deletions gorush/notification_apns.go
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ func getApnsClient(req PushNotification) (client *apns2.Client) {
}

// PushToIOS provide send notification to APNs server.
func PushToIOS(req PushNotification) bool {
func PushToIOS(req PushNotification) {
LogAccess.Debug("Start push notification for iOS")

var (
Expand All @@ -374,7 +374,6 @@ func PushToIOS(req PushNotification) bool {

Retry:
var (
isError = false
newTokens []string
)

Expand All @@ -386,12 +385,11 @@ Retry:
// occupy push slot
MaxConcurrentIOSPushes <- struct{}{}
wg.Add(1)
go func(token string) {
go func(notification apns2.Notification, token string) {
notification.DeviceToken = token

// send ios notification
res, err := client.Push(notification)

res, err := client.Push(&notification)
if err != nil || (res != nil && res.StatusCode != http.StatusOK) {
if err == nil {
// error message:
Expand All @@ -418,27 +416,24 @@ Retry:
if res != nil && res.StatusCode >= http.StatusInternalServerError {
newTokens = append(newTokens, token)
}
isError = true
}

if res != nil && res.Sent() && !isError {
if res != nil && res.Sent() {
LogPush(SucceededPush, token, req, nil)
StatStorage.AddIosSuccess(1)
}
// free push slot
<-MaxConcurrentIOSPushes
wg.Done()
}(token)
}(*notification, token)
}
wg.Wait()

if isError && retryCount < maxRetry {
if len(newTokens) > 0 && retryCount < maxRetry {
retryCount++

// resend fail token
req.Tokens = newTokens
goto Retry
}

return isError
}
5 changes: 2 additions & 3 deletions gorush/notification_apns_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -706,14 +706,13 @@ func TestPushToIOS(t *testing.T) {
assert.Nil(t, err)

req := PushNotification{
Tokens: []string{"11aa01229f15f0f0c52029d8cf8cd0aeaf2365fe4cebc4af26cd6d76b7919ef7"},
Tokens: []string{"11aa01229f15f0f0c52029d8cf8cd0aeaf2365fe4cebc4af26cd6d76b7919ef7", "11aa01229f15f0f0c52029d8cf8cd0aeaf2365fe4cebc4af26cd6d76b7919ef1"},
Platform: 1,
Message: "Welcome",
}

// send fail
isError := PushToIOS(req)
assert.True(t, isError)
PushToIOS(req)
}

func TestApnsHostFromRequest(t *testing.T) {
Expand Down
17 changes: 5 additions & 12 deletions gorush/notification_fcm.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func GetAndroidNotification(req PushNotification) *fcm.Message {
}

// PushToAndroid provide send notification to Android server.
func PushToAndroid(req PushNotification) bool {
func PushToAndroid(req PushNotification) {
LogAccess.Debug("Start push notification for Android")

var (
Expand All @@ -119,12 +119,10 @@ func PushToAndroid(req PushNotification) bool {

if err != nil {
LogError.Error("request error: " + err.Error())
return false
return
}

Retry:
var isError = false

notification := GetAndroidNotification(req)

if req.APIKey != "" {
Expand All @@ -136,14 +134,14 @@ Retry:
if err != nil {
// FCM server error
LogError.Error("FCM server error: " + err.Error())
return false
return
}

res, err := client.Send(notification)
if err != nil {
// Send Message error
LogError.Error("FCM server send message error: " + err.Error())
return false
return
}

if !req.IsTopic() {
Expand All @@ -169,7 +167,6 @@ Retry:
if !result.Unregistered() {
newTokens = append(newTokens, to)
}
isError = true

LogPush(FailedPush, to, req, result.Error)
if PushConf.Core.Sync {
Expand Down Expand Up @@ -201,7 +198,6 @@ Retry:
if res.MessageID != 0 {
LogPush(SucceededPush, to, req, nil)
} else {
isError = true
// failure
LogPush(FailedPush, to, req, res.Error)
if PushConf.Core.Sync {
Expand All @@ -212,7 +208,6 @@ Retry:

// Device Group HTTP Response
if len(res.FailedRegistrationIDs) > 0 {
isError = true
newTokens = append(newTokens, res.FailedRegistrationIDs...)

LogPush(FailedPush, notification.To, req, errors.New("device group: partial success or all fails"))
Expand All @@ -221,13 +216,11 @@ Retry:
}
}

if isError && retryCount < maxRetry {
if len(newTokens) > 0 && retryCount < maxRetry {
retryCount++

// resend fail token
req.Tokens = newTokens
goto Retry
}

return isError
}
15 changes: 5 additions & 10 deletions gorush/notification_fcm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,7 @@ func TestPushToAndroidWrongToken(t *testing.T) {
}

// Android Success count: 0, Failure count: 2
isError := PushToAndroid(req)
assert.True(t, isError)
PushToAndroid(req)
}

func TestPushToAndroidRightTokenForJSONLog(t *testing.T) {
Expand All @@ -80,8 +79,7 @@ func TestPushToAndroidRightTokenForJSONLog(t *testing.T) {
Message: "Welcome",
}

isError := PushToAndroid(req)
assert.False(t, isError)
PushToAndroid(req)
}

func TestPushToAndroidRightTokenForStringLog(t *testing.T) {
Expand All @@ -98,8 +96,7 @@ func TestPushToAndroidRightTokenForStringLog(t *testing.T) {
Message: "Welcome",
}

isError := PushToAndroid(req)
assert.False(t, isError)
PushToAndroid(req)
}

func TestOverwriteAndroidAPIKey(t *testing.T) {
Expand All @@ -119,8 +116,7 @@ func TestOverwriteAndroidAPIKey(t *testing.T) {
}

// FCM server error: 401 error: 401 Unauthorized (Wrong API Key)
err := PushToAndroid(req)
assert.False(t, err)
PushToAndroid(req)
}

func TestFCMMessage(t *testing.T) {
Expand Down Expand Up @@ -215,8 +211,7 @@ func TestCheckAndroidMessage(t *testing.T) {
TimeToLive: &timeToLive,
}

err := PushToAndroid(req)
assert.False(t, err)
PushToAndroid(req)
}

func TestAndroidNotificationStructure(t *testing.T) {
Expand Down