Skip to content

fix(apns): copy client per request to avoid shared host mutation - #882

Open
ridwanakf wants to merge 1 commit into
appleboy:masterfrom
ridwanakf:fix/apns-client-host-race
Open

fix(apns): copy client per request to avoid shared host mutation#882
ridwanakf wants to merge 1 commit into
appleboy:masterfrom
ridwanakf:fix/apns-client-host-race

Conversation

@ridwanakf

Copy link
Copy Markdown

Fixes #881.

Opening this proactively with a fix. If mutating the shared client is intentional and I have misread the send path, happy to close.

I went looking at why some iOS pushes can land in the wrong APNs environment. It comes down to getApnsClient: it picks the environment by calling Production() / Development() on the shared global ApnsClient, and in apns2 those two methods mutate the receiver and return it rather than handing back a configured copy. So every call rewrites ApnsClient.Host on the single process-wide client and returns that same pointer.

Once more than one push is in flight (and worker_num defaults to runtime.NumCPU(), so that is the normal case) two things go wrong:

  1. It is a data race on ApnsClient.Host. It happens even when both requests want the same host, because both still write the field.
  2. When two requests disagree, the last writer wins for everyone. PushToIOS resolves the client once and then fans out into goroutines that each read the host when they build their request, so a later request can redirect pushes that were already in flight. A production: true notification can go to the sandbox, and a development: true one to production.

It is easy to miss in practice because Apple just returns a generic BadDeviceToken for a wrong-environment token, so it looks like ordinary token churn and the rate tracks your traffic mix. The full write-up, a deterministic reproduction, and the -race output are in #881.

The fix is small: copy the client per request and set the host on the copy, so the global is never touched.

client := *ApnsClient
switch {
case req.Production:
	client.Host = apns2.HostProduction
case req.Development:
	client.Host = apns2.HostDevelopment
default:
	if cfg.Ios.Production {
		client.Host = apns2.HostProduction
	} else {
		client.Host = apns2.HostDevelopment
	}
}
return &client

apns2.Client is a plain struct with no lock, and its HTTPClient and Token are safe to share, so the copy is cheap and keeps the same connection pool.

Tests live in notify/notification_apns_host_test.go:

  • host selection for both the per-request override and the config default,
  • a regression test that a later development request no longer repoints an earlier production client (it fails on master today),
  • a concurrent test that stays clean under -race.

I kept this to getApnsClient alone. Running the whole package under -race also turns up a separate, older race on the resp.Logs / newTokens appends in PushToIOS, but that one is already being handled in #874, so I left it out here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Concurrent iOS pushes can be delivered to the wrong APNs host

1 participant