Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions config/mattermost-push-proxy.sample.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"ThrottleVaryByHeader":"X-Forwarded-For",
"EnableMetrics": false,
"SendTimeoutSec": 30,
"RetryTimeoutSec": 8,
"ApplePushSettings":[
{
"Type":"apple",
Expand Down
61 changes: 52 additions & 9 deletions server/apple_notification_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,16 @@ type AppleNotificationServer struct {
logger *Logger
ApplePushSettings ApplePushSettings
sendTimeout time.Duration
retryTimeout time.Duration
}

func NewAppleNotificationServer(settings ApplePushSettings, logger *Logger, metrics *metrics, sendTimeoutSecs int) *AppleNotificationServer {
func NewAppleNotificationServer(settings ApplePushSettings, logger *Logger, metrics *metrics, sendTimeoutSecs int, retryTimeoutSecs int) *AppleNotificationServer {
return &AppleNotificationServer{
ApplePushSettings: settings,
metrics: metrics,
logger: logger,
sendTimeout: time.Duration(sendTimeoutSecs) * time.Second,
retryTimeout: time.Duration(retryTimeoutSecs) * time.Second,
}
}

Expand Down Expand Up @@ -227,15 +229,8 @@ func (me *AppleNotificationServer) SendNotification(msg *PushNotification) PushR

if me.AppleClient != nil {
me.logger.Infof("Sending apple push notification for device=%v type=%v ackId=%v", me.ApplePushSettings.Type, msg.Type, msg.AckID)
start := time.Now()

ctx, cancel := context.WithTimeout(context.Background(), me.sendTimeout)
defer cancel()

res, err := me.AppleClient.PushWithContext(ctx, notification)
if me.metrics != nil {
me.metrics.observerNotificationResponse(PushNotifyApple, time.Since(start).Seconds())
}
res, err := me.SendNotificationWithRetry(notification)
if err != nil {
me.logger.Errorf("Failed to send apple push sid=%v did=%v err=%v type=%v", msg.ServerID, msg.DeviceID, err, me.ApplePushSettings.Type)
if me.metrics != nil {
Expand Down Expand Up @@ -269,3 +264,51 @@ func (me *AppleNotificationServer) SendNotification(msg *PushNotification) PushR
}
return NewOkPushResponse()
}

func (me *AppleNotificationServer) SendNotificationWithRetry(notification *apns.Notification) (*apns.Response, error) {
var res *apns.Response
var err error
waitTime := time.Second

// Keep a general context to make sure the whole retry
// doesn't take longer than the timeout.
generalContext, cancelGeneralContext := context.WithTimeout(context.Background(), me.sendTimeout)
defer cancelGeneralContext()

for retries := 0; retries < MAX_RETRIES; retries++ {
start := time.Now()

retryContext, cancelRetryContext := context.WithTimeout(generalContext, me.retryTimeout)
defer cancelRetryContext()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor note: having defers in a for loop has a slight problem that it will keep getting accumulated until the whole function exits. But since this loop has a hardcoded upper bound, this is okay. Otherwise, it would be recommended to cancel as soon as one iteration finishes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For readability sake, I feel it is better to keep it as is. As you said, the loop is bound, so it should not be a big issue.

res, err = me.AppleClient.PushWithContext(retryContext, notification)
if me.metrics != nil {
me.metrics.observerNotificationResponse(PushNotifyApple, time.Since(start).Seconds())
}

if err == nil {
break
}

me.logger.Errorf("Failed to send apple push did=%v retry=%v error=%v", notification.DeviceToken, retries, err)

if retries == MAX_RETRIES-1 {
me.logger.Errorf("Max retries reached did=%v", notification.DeviceToken)
break
}

select {
case <-generalContext.Done():
case <-time.After(waitTime):
}

if generalContext.Err() != nil {
me.logger.Infof("Not retrying because context error did=%v retry=%v error=%v", notification.DeviceToken, retries, generalContext.Err())
err = generalContext.Err()
break
}

waitTime *= 2
}

return res, err
}
11 changes: 11 additions & 0 deletions server/config_push_proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ type ConfigPushProxy struct {
ThrottleVaryByHeader string
LogFileLocation string
SendTimeoutSec int
RetryTimeoutSec int
ApplePushSettings []ApplePushSettings
EnableMetrics bool
EnableConsoleLog bool
Expand Down Expand Up @@ -81,6 +82,16 @@ func LoadConfig(fileName string) (*ConfigPushProxy, error) {
if !cfg.EnableConsoleLog && !cfg.EnableFileLog {
cfg.EnableConsoleLog = true
}

// Set timeout defaults
if cfg.SendTimeoutSec == 0 {
cfg.SendTimeoutSec = 30
}

if cfg.RetryTimeoutSec == 0 {
cfg.RetryTimeoutSec = 8
}

if cfg.EnableFileLog {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a check here to verify that the retryTimeout isn't greater than the sendTimeout.

if cfg.LogFileLocation == "" {
// We just do an mkdir -p equivalent.
Expand Down
3 changes: 2 additions & 1 deletion server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const (
HEADER_REAL_IP = "X-Real-IP"
WAIT_FOR_SERVER_SHUTDOWN = time.Second * 5
CONNECTION_TIMEOUT_SECONDS = 60
MAX_RETRIES = 3
)

type NotificationServer interface {
Expand Down Expand Up @@ -69,7 +70,7 @@ func (s *Server) Start() {
}

for _, settings := range s.cfg.ApplePushSettings {
server := NewAppleNotificationServer(settings, s.logger, m, s.cfg.SendTimeoutSec)
server := NewAppleNotificationServer(settings, s.logger, m, s.cfg.SendTimeoutSec, s.cfg.RetryTimeoutSec)
err := server.Initialize()
if err != nil {
s.logger.Errorf("Failed to initialize client: %v", err)
Expand Down
Loading