chore: fix race condition with push iOS notification (#533)

appleboy · Jul 14, 2020 · a4cc8d7 · a4cc8d7
1 parent 7c7e740
commit a4cc8d7
Show file tree

Hide file tree

Showing 4 changed files with 18 additions and 36 deletions.
diff --git a/gorush/notification_apns.go b/gorush/notification_apns.go
@@ -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 (
@@ -374,7 +374,6 @@ func PushToIOS(req PushNotification) bool {
 
 Retry:
 	var (
-		isError   = false
 		newTokens []string
 	)
 
@@ -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:
@@ -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
 }
diff --git a/gorush/notification_apns_test.go b/gorush/notification_apns_test.go
@@ -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) {

diff --git a/gorush/notification_fcm.go b/gorush/notification_fcm.go
@@ -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 (
@@ -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 != "" {
@@ -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() {
@@ -169,7 +167,6 @@ Retry:
 			if !result.Unregistered() {
 				newTokens = append(newTokens, to)
 			}
-			isError = true
 
 			LogPush(FailedPush, to, req, result.Error)
 			if PushConf.Core.Sync {
@@ -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 {
@@ -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"))
@@ -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
 }
diff --git a/gorush/notification_fcm_test.go b/gorush/notification_fcm_test.go
@@ -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) {
@@ -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) {
@@ -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) {
@@ -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) {
@@ -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) {