-
Notifications
You must be signed in to change notification settings - Fork 765
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Agma Module Shutdown() test #3844
base: master
Are you sure you want to change the base?
Conversation
glog.Warning("[AgmaAnalytics] fail to copy the buffer") | ||
return | ||
} | ||
|
||
go l.sender(payload) | ||
|
||
l.reset() | ||
l.mux.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that payload
is a copy, l.reset()
and l.mux.Unlock()
should be moved upstream so the memory is unlocked and available for other processes in case go l.sender(payload)
gets run before the remainder of flush()
.
@@ -65,8 +66,11 @@ func newAgmaLogger(cfg config.AgmaAnalytics, sender httpSender, clock clock.Cloc | |||
maxEventCount: int64(cfg.Buffers.EventCount), | |||
maxDuration: pDuration, | |||
buffer: buffer, | |||
bufferCh: make(chan []byte), | |||
sigTermCh: make(chan os.Signal, 1), | |||
read: func(p []byte, b bytes.Buffer) (int, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be more flexible, and make more sense, to make the buffer settable (an input parameter) by the newAgmaLogger()
call. For the test you can make a mock buffer (it would have to implement the full buffer interface, which is a little more work) with your reader method as the mock buffer's read method. This would also allow for the mock buffer to be used in other tests.
Test
TestShutdownFlush
sometimes fails theAssertCalled()
check when run from thevalidate.sh
script due to a race condition that thetime.Sleep()
call in line 697 is unable to avoid if the goroutinego l.sender(payload)
insideflush()
takes longer to get run by golang's goroutine scheduler than the 10 millisecond wait.This pull request refactors
TestShutdownFlush
so nogo l.sender(payload)
goroutine gets called so the occurence of a race condition is less likely.