From 0792e48e6fee68aa535f8e66a9cd791d713b3e6c Mon Sep 17 00:00:00 2001 From: Bo-Yi Wu Date: Sat, 6 Jun 2026 17:07:37 +0800 Subject: [PATCH 1/2] refactor: simplify push and logging hot paths - Extract a shared retry-clamp helper for the iOS, Android, and Huawei push functions - Merge the duplicate HTTP and HTTPS server startup helpers into one - Build push log output only when the destination log level is enabled - Batch per-token iOS success and error counters into a single stat update - Preallocate FCM message and failure log slices to their known size - Parse feedback headers with strings.Cut so values containing a colon are kept - Drop a no-op topic reassignment and an unused misspelled priority constant - Collapse redundant error-return blocks and a verbose topic check --- app/sender.go | 21 +++------- logx/log.go | 80 ++++++++++++++++++++----------------- notify/feedback.go | 7 ++-- notify/global.go | 3 +- notify/notification.go | 14 +++++-- notify/notification_apns.go | 30 +++++++++----- notify/notification_fcm.go | 11 ++--- notify/notification_hms.go | 6 +-- router/server.go | 2 +- router/server_normal.go | 33 +++++---------- rpc/server.go | 5 --- 11 files changed, 100 insertions(+), 112 deletions(-) diff --git a/app/sender.go b/app/sender.go index 5ccbe0aee..c90312eb5 100644 --- a/app/sender.go +++ b/app/sender.go @@ -40,11 +40,8 @@ func SendAndroidNotification(ctx context.Context, cfg *config.ConfYaml, opts CLI return err } - if _, err := notify.PushToAndroid(ctx, req, cfg); err != nil { - return err - } - - return nil + _, err := notify.PushToAndroid(ctx, req, cfg) + return err } // SendHuaweiNotification sends a Huawei notification via CLI. @@ -73,11 +70,8 @@ func SendHuaweiNotification(ctx context.Context, cfg *config.ConfYaml, opts CLIS return err } - if _, err := notify.PushToHuawei(ctx, req, cfg); err != nil { - return err - } - - return nil + _, err := notify.PushToHuawei(ctx, req, cfg) + return err } // SendIOSNotification sends an iOS notification via CLI. @@ -110,11 +104,8 @@ func SendIOSNotification(ctx context.Context, cfg *config.ConfYaml, opts CLISend return err } - if _, err := notify.PushToIOS(ctx, req, cfg); err != nil { - return err - } - - return nil + _, err := notify.PushToIOS(ctx, req, cfg) + return err } // SendNotification sends a notification based on platform type. diff --git a/logx/log.go b/logx/log.go index 5ee02d601..fe0bd273f 100644 --- a/logx/log.go +++ b/logx/log.go @@ -205,54 +205,62 @@ type InputLog struct { // LogPush record user push request and server response. func LogPush(input *InputLog) LogPushEntry { - var platColor, resetColor, output string + log := GetLogPushEntry(input) - if isTerm { - platColor = colorForPlatForm(input.Platform) - resetColor = reset + // Only build the output string when the destination logger will actually + // emit it; the marshal/format runs once per token on the push hot path. + switch input.Status { + case core.SucceededPush: + if LogAccess.IsLevelEnabled(logrus.InfoLevel) { + LogAccess.Info(formatPushLog(input, log)) + } + case core.FailedPush: + if LogError.IsLevelEnabled(logrus.ErrorLevel) { + LogError.Error(formatPushLog(input, log)) + } } - log := GetLogPushEntry(input) + return log +} +// formatPushLog renders a push log entry as JSON or colored text. +func formatPushLog(input *InputLog, log LogPushEntry) string { if input.Format == "json" { logJSON, _ := json.Marshal(log) + return string(logJSON) + } - output = string(logJSON) - } else { - var typeColor string - switch input.Status { - case core.SucceededPush: - if isTerm { - typeColor = green - } - - output = fmt.Sprintf("|%s %s %s| %s%s%s [%s] %s", - typeColor, log.Type, resetColor, - platColor, log.Platform, resetColor, - log.Token, - log.Message, - ) - case core.FailedPush: - if isTerm { - typeColor = red - } - - output = fmt.Sprintf("|%s %s %s| %s%s%s [%s] | %s | Error Message: %s", - typeColor, log.Type, resetColor, - platColor, log.Platform, resetColor, - log.Token, - log.Message, - log.Error, - ) - } + var platColor, resetColor, typeColor string + if isTerm { + platColor = colorForPlatForm(input.Platform) + resetColor = reset } switch input.Status { case core.SucceededPush: - LogAccess.Info(output) + if isTerm { + typeColor = green + } + + return fmt.Sprintf("|%s %s %s| %s%s%s [%s] %s", + typeColor, log.Type, resetColor, + platColor, log.Platform, resetColor, + log.Token, + log.Message, + ) case core.FailedPush: - LogError.Error(output) + if isTerm { + typeColor = red + } + + return fmt.Sprintf("|%s %s %s| %s%s%s [%s] | %s | Error Message: %s", + typeColor, log.Type, resetColor, + platColor, log.Platform, resetColor, + log.Token, + log.Message, + log.Error, + ) } - return log + return "" } diff --git a/notify/feedback.go b/notify/feedback.go index 77eb38f8f..b4983b31c 100644 --- a/notify/feedback.go +++ b/notify/feedback.go @@ -13,11 +13,10 @@ import ( // extractHeaders converts a slice of strings to a map of strings. func extractHeaders(headers []string) map[string]string { - result := make(map[string]string) + result := make(map[string]string, len(headers)) for _, header := range headers { - parts := strings.Split(header, ":") - if len(parts) == 2 { - result[parts[0]] = parts[1] + if key, value, ok := strings.Cut(header, ":"); ok { + result[key] = value } } return result diff --git a/notify/global.go b/notify/global.go index 88eec445c..e8d62de4c 100644 --- a/notify/global.go +++ b/notify/global.go @@ -36,6 +36,5 @@ var ( ) const ( - HIGH = "high" - NORMAL = "nornal" + HIGH = "high" ) diff --git a/notify/notification.go b/notify/notification.go index 4798cc9b2..b8a2af3ca 100644 --- a/notify/notification.go +++ b/notify/notification.go @@ -142,11 +142,17 @@ func (p *PushNotification) Payload() []byte { // IsTopic check if message format is topic for FCM // ref: https://firebase.google.com/docs/cloud-messaging/send-message#topic-http-post-request func (p *PushNotification) IsTopic() bool { - if p.Platform == core.PlatFormHuawei || p.Platform == core.PlatFormAndroid { - return p.Topic != "" || p.Condition != "" - } + return (p.Platform == core.PlatFormHuawei || p.Platform == core.PlatFormAndroid) && + (p.Topic != "" || p.Condition != "") +} - return false +// effectiveMaxRetry clamps the configured retry count by the per-request Retry +// override when the request asks for fewer retries. +func effectiveMaxRetry(reqRetry, cfgMax int) int { + if reqRetry > 0 && reqRetry < cfgMax { + return reqRetry + } + return cfgMax } // CheckMessage for check request message diff --git a/notify/notification_apns.go b/notify/notification_apns.go index 2c14ada38..8202a4174 100644 --- a/notify/notification_apns.go +++ b/notify/notification_apns.go @@ -10,6 +10,7 @@ import ( "net/http" "path/filepath" "sync" + "sync/atomic" "time" "github.com/appleboy/gorush/config" @@ -467,19 +468,17 @@ func PushToIOS( ) (resp *ResponsePush, err error) { logx.LogAccess.Debug("Start push notification for iOS") - var ( - retryCount = 0 - maxRetry = cfg.Ios.MaxRetry - ) - - if req.Retry > 0 && req.Retry < maxRetry { - maxRetry = req.Retry - } + retryCount := 0 + maxRetry := effectiveMaxRetry(req.Retry, cfg.Ios.MaxRetry) resp = &ResponsePush{} Retry: - var newTokens []string + var ( + newTokens []string + successCount atomic.Int64 + errorCount atomic.Int64 + ) notification := GetIOSNotification(req) client := getApnsClient(cfg, req) @@ -505,7 +504,7 @@ Retry: errLog := logPush(cfg, core.FailedPush, token, req, err) resp.Logs = append(resp.Logs, errLog) - status.StatStorage.AddIosError(1) + errorCount.Add(1) // We should retry only "retryable" statuses. More info about response: // See https://apple.co/3AdNane (Handling Notification Responses from APNs) if res != nil && res.StatusCode >= http.StatusInternalServerError { @@ -515,7 +514,7 @@ Retry: if res != nil && res.Sent() { logPush(cfg, core.SucceededPush, token, req, nil) - status.StatStorage.AddIosSuccess(1) + successCount.Add(1) } // free push slot @@ -526,6 +525,15 @@ Retry: wg.Wait() + // Flush the per-token counts in a single update each, instead of one + // stat-storage round-trip per device token. + if n := successCount.Load(); n > 0 { + status.StatStorage.AddIosSuccess(n) + } + if n := errorCount.Load(); n > 0 { + status.StatStorage.AddIosError(n) + } + if len(newTokens) > 0 && retryCount < maxRetry { retryCount++ diff --git a/notify/notification_fcm.go b/notify/notification_fcm.go index 594fccd3f..3340b9947 100644 --- a/notify/notification_fcm.go +++ b/notify/notification_fcm.go @@ -148,9 +148,9 @@ func convertDataToStringMap(data map[string]any) map[string]string { } result := make(map[string]string, len(data)) for k, v := range data { - switch v.(type) { + switch val := v.(type) { case string: - result[k] = fmt.Sprintf("%s", v) + result[k] = val default: if b, err := json.Marshal(v); err == nil { result[k] = string(b) @@ -184,7 +184,7 @@ func GetAndroidNotification(req *PushNotification) []*messaging.Message { setupFCMSound(req) data := convertDataToStringMap(req.Data) - var messages []*messaging.Message + messages := make([]*messaging.Message, 0, len(req.Tokens)+1) if req.IsTopic() { msg := buildFCMMessage(req, data) @@ -277,10 +277,7 @@ func PushToAndroid( return nil, err } - maxRetry := cfg.Android.MaxRetry - if req.Retry > 0 && req.Retry < maxRetry { - maxRetry = req.Retry - } + maxRetry := effectiveMaxRetry(req.Retry, cfg.Android.MaxRetry) resp = &ResponsePush{} retryCount := 0 diff --git a/notify/notification_hms.go b/notify/notification_hms.go index 585e27b8e..87ea47c50 100644 --- a/notify/notification_hms.go +++ b/notify/notification_hms.go @@ -166,13 +166,9 @@ func PushToHuawei( var ( client *client.HMSClient retryCount = 0 - maxRetry = cfg.Huawei.MaxRetry + maxRetry = effectiveMaxRetry(req.Retry, cfg.Huawei.MaxRetry) ) - if req.Retry > 0 && req.Retry < maxRetry { - maxRetry = req.Retry - } - // check message err = CheckMessage(req) if err != nil { diff --git a/router/server.go b/router/server.go index d56e20ce8..39bcc5b44 100644 --- a/router/server.go +++ b/router/server.go @@ -227,7 +227,7 @@ func markFailedNotification( reason string, ) []logx.LogPushEntry { logx.LogError.Error(reason) - logs := make([]logx.LogPushEntry, 0) + logs := make([]logx.LogPushEntry, 0, len(notification.Tokens)) for _, token := range notification.Tokens { logs = append(logs, logx.GetLogPushEntry(&logx.InputLog{ ID: notification.ID, diff --git a/router/server_normal.go b/router/server_normal.go index c47f6c2db..8e352cc4e 100644 --- a/router/server_normal.go +++ b/router/server_normal.go @@ -86,25 +86,12 @@ func RunHTTPServer( return startServer(ctx, server, cfg) } -func listenAndServe(ctx context.Context, s *http.Server, cfg *config.ConfYaml) error { - var g errgroup.Group - g.Go(func() error { - <-ctx.Done() - timeout := time.Duration(cfg.Core.ShutdownTimeout) * time.Second - ctx, cancel := context.WithTimeout(context.Background(), timeout) - defer cancel() - return s.Shutdown(ctx) - }) - g.Go(func() error { - if err := s.ListenAndServe(); err != nil && err != http.ErrServerClosed { - return err - } - return nil - }) - return g.Wait() -} - -func listenAndServeTLS(ctx context.Context, s *http.Server, cfg *config.ConfYaml) error { +func serveUntilShutdown( + ctx context.Context, + s *http.Server, + cfg *config.ConfYaml, + serve func() error, +) error { var g errgroup.Group g.Go(func() error { <-ctx.Done() @@ -114,7 +101,7 @@ func listenAndServeTLS(ctx context.Context, s *http.Server, cfg *config.ConfYaml return s.Shutdown(ctx) }) g.Go(func() error { - if err := s.ListenAndServeTLS("", ""); err != nil && err != http.ErrServerClosed { + if err := serve(); err != nil && err != http.ErrServerClosed { return err } return nil @@ -124,8 +111,10 @@ func listenAndServeTLS(ctx context.Context, s *http.Server, cfg *config.ConfYaml func startServer(ctx context.Context, s *http.Server, cfg *config.ConfYaml) error { if s.TLSConfig == nil { - return listenAndServe(ctx, s, cfg) + return serveUntilShutdown(ctx, s, cfg, s.ListenAndServe) } - return listenAndServeTLS(ctx, s, cfg) + return serveUntilShutdown(ctx, s, cfg, func() error { + return s.ListenAndServeTLS("", "") + }) } diff --git a/rpc/server.go b/rpc/server.go index 7df5200ca..f73943b12 100644 --- a/rpc/server.go +++ b/rpc/server.go @@ -13,7 +13,6 @@ import ( "firebase.google.com/go/v4/messaging" "github.com/appleboy/gorush/config" - "github.com/appleboy/gorush/core" "github.com/appleboy/gorush/logx" "github.com/appleboy/gorush/notify" "github.com/appleboy/gorush/rpc/proto" @@ -94,10 +93,6 @@ func (s *Server) Send( notification.Badge = &badge } - if in.Topic != "" && in.Platform == core.PlatFormAndroid { - notification.Topic = in.Topic - } - if in.Alert != nil { notification.Alert = notify.Alert{ Title: in.Alert.Title, From faf3a5dd7a274ab0c56838a8f588d3e786e91f11 Mon Sep 17 00:00:00 2001 From: Bo-Yi Wu Date: Sun, 7 Jun 2026 10:57:32 +0800 Subject: [PATCH 2/2] fix(notify): guard concurrent iOS push slice appends with a mutex The per-token goroutines in PushToIOS appended to the shared resp.Logs and newTokens slices without synchronization. With MaxConcurrentPushes defaulting to 100, many goroutines run in parallel and race on the slice headers, which can drop log entries, drop retryable tokens, or panic on a backing-array reallocation. The prior change made the stat counters atomic but left these two appends unguarded; add a mutex around both. Co-Authored-By: Claude Opus 4.8 (1M context) --- notify/notification_apns.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/notify/notification_apns.go b/notify/notification_apns.go index 8202a4174..8b817b935 100644 --- a/notify/notification_apns.go +++ b/notify/notification_apns.go @@ -478,6 +478,7 @@ Retry: newTokens []string successCount atomic.Int64 errorCount atomic.Int64 + mu sync.Mutex // guards newTokens and resp.Logs against the per-token goroutines ) notification := GetIOSNotification(req) @@ -502,14 +503,17 @@ Retry: // apns server error errLog := logPush(cfg, core.FailedPush, token, req, err) - resp.Logs = append(resp.Logs, errLog) - errorCount.Add(1) // We should retry only "retryable" statuses. More info about response: // See https://apple.co/3AdNane (Handling Notification Responses from APNs) - if res != nil && res.StatusCode >= http.StatusInternalServerError { + retryable := res != nil && res.StatusCode >= http.StatusInternalServerError + + mu.Lock() + resp.Logs = append(resp.Logs, errLog) + if retryable { newTokens = append(newTokens, token) } + mu.Unlock() } if res != nil && res.Sent() {