refactor: simplify push and logging hot paths#874
Open
appleboy wants to merge 2 commits into
Open
Conversation
- 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
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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Behavior-preserving cleanup of the push and logging code surfaced by a
/simplifyreview across the codebase. Removes duplication (a shared retry-clamp helper, merged HTTP/HTTPS startup), trims wasted work on the per-token hot path (log output built only when the level is enabled, iOS stat counters batched into one update, slices preallocated), and collapses redundant code. No external behavior or public API changes.Architecture / flow
The only runtime-shape change worth a picture is the iOS push loop in
notify/notification_apns.go: per-token goroutines previously hit stat storage once per token; they now accumulate into atomic counters and flush once after the wait barrier (identical totals, far fewer stat-storage round-trips on the Redis backend).flowchart TD Start[PushToIOS batch] --> Spawn[spawn goroutine per token] Spawn --> Push[client.PushWithContext] Push --> Ok{sent ok?} Ok -- yes --> Sinc[successCount.Add 1] Ok -- no --> Einc[errorCount.Add 1 + append retry token] Sinc --> Wait[wg.Wait barrier] Einc --> Wait Wait --> Flush[single AddIosSuccess / AddIosError flush] Flush --> Retry{retryable tokens left?} Retry -- yes --> Start Retry -- no --> Done[return resp] style Sinc fill:#dff0d8,stroke:#3c763d style Einc fill:#dff0d8,stroke:#3c763d style Flush fill:#dff0d8,stroke:#3c763dAI Authorship
/simplifyworkflowChange classification
Touches the shared push path (
notify/) and logging (logx/). The changes are behavior-preserving, but these modules are central, so the stricter review bar applies.Plan reference
No plan doc — this is a follow-up cleanup from a
/simplifyreview. Goal: reduce duplication and wasted work without changing behavior.Verification
go test -tags sqlite ./...)notify/routertests fail in this environment withmissing fcm credential data; confirmed pre-existing (they fail identically on the unchanged base tree — they requireFCM_CREDENTIAL/FCM_TEST_TOKEN)go build -tags sqlite ./...— successgo vet -tags sqlite ./...— no issuesgofmt -lon changed dirs — cleanVerifiability check
Addper outcome → same sum)Risk & rollback
Reviewer guide
notify/notification_apns.go— atomic counters + post-wg.Wait()flush (concurrency)logx/log.go—LogPushsplit intoformatPushLog, gated onIsLevelEnabled(confirm output identical for json + text, succeeded + failed)effectiveMaxRetryhelper + its three call sitesrouter/server_normal.gomerged startup helpernotify/feedback.gostrings.Cut,rpc/server.gono-op removal,notify/global.goconstant removal,app/sender.goreturn errcollapse, slice preallocations🤖 Generated with Claude Code