Fix concurrency issues and data race on tests #53
Conversation
extensions/apns_message_handler.go
Outdated
| @@ -411,7 +415,7 @@ func (a *APNSMessageHandler) LogStats() { | |||
| func (a *APNSMessageHandler) mapErrorReason(reason string) string { | |||
| switch reason { | |||
| case apns2.ReasonPayloadEmpty: | |||
| return "payload-empty" | |||
There was a problem hiding this comment.
No. Fixed.
extensions/apns_message_handler.go
Outdated
| @@ -411,7 +415,7 @@ func (a *APNSMessageHandler) LogStats() { | |||
| func (a *APNSMessageHandler) mapErrorReason(reason string) string { | |||
| switch reason { | |||
| case apns2.ReasonPayloadEmpty: | |||
| return "payload-empty" | |||
There was a problem hiding this comment.
It was not intentional
| } | ||
|
|
||
| //func (s *GCMMessageHandlerTestSuite) SetupSubTest() { |
There was a problem hiding this comment.
Do we still need this? Can't we just remove?
There was a problem hiding this comment.
Removed
| s.Equal(int64(1), handler.sentMessages) | ||
| s.Equal(int64(0), handler.ignoredMessages) |
There was a problem hiding this comment.
Don't we need to acquire/release lock here?
feedback/invalid_token.go
Outdated
|
|
||
| i.BufferLock.Lock() |
There was a problem hiding this comment.
Why we don't use the same .Lock() + .Unlock block for appending and clearing the buffer? Is i.deleteTokens a costly operation?
There was a problem hiding this comment.
No actual reason. Set everything to one lock/unlock.
feedback/invalid_token.go
Outdated
| if len(i.Buffer) >= i.bufferSize { | ||
| l.Debug("buffer is full") | ||
| i.deleteTokens(i.Buffer) | ||
| i.Buffer = make([]*InvalidToken, 0, i.bufferSize) | ||
| } | ||
| i.BufferLock.Unlock() | ||
| } | ||
|
|
||
| case <-flushTicker.C: | ||
| l.Debug("flush ticker") | ||
| i.deleteTokens(i.Buffer) |
There was a problem hiding this comment.
shouldn't we guard deleteTokens under the .Lock() since we're changing i.Buffer?
There was a problem hiding this comment.
True. Good catch!
No description provided.