-
Notifications
You must be signed in to change notification settings - Fork 345
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 SIGSEGV with zstd compression enabled #1164
Conversation
Confirmed with a manual test that this PR fixes the issue for us. Thanks @RobertIndie! |
@@ -40,7 +40,7 @@ func TimestampMillis(t time.Time) uint64 { | |||
// GetAndAdd perform atomic read and update | |||
func GetAndAdd(n *uint64, diff uint64) uint64 { | |||
for { | |||
v := *n | |||
v := atomic.LoadUint64(n) | |||
if atomic.CompareAndSwapUint64(n, v, v+diff) { |
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'm not familiar enough with the pulsar codebase to understand what this is trying to achieve, but the usual way to perform an atomic get-and-add on a uint64 in golang is to use atomic.AddUint64()
. Also, is this change related to the issue at hand, or is it an unrelated change?
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.
This change is to fix a race issue in the CI discovered by the new test case TestSendConcurrently
.
The difference between them is that atomic.AddUint64() returns the new value but GetAndAdd
here returns the old value.
Actually I was trying to use atomic.AddUint64(), but if failed in the CI because there are other cases that need the old values.
And yes, this is to fix another regression bug introduced in 0.12.0.
The updated version works for us -- thanks again @RobertIndie. |
* Fix SIGSEGV with zstd compression enabled * Use sync.Pool to cache zstd ctx * Fix race in sequenceID assignment * Fix GetAndAdd (cherry picked from commit 8776135)
Fixes #1163
Thanks @0x4500 for reporting this issue and providing the analysis.
Motivation
#1121 introduces a regression bug. The compression logic has been moved from
internalSend
tointernalSendAsync
which leads to the compression being executed concurrently.However,
zstd_cgo
doesn't support concurrent compression of data. To prevent this, we need to introduce a mutex.Modifications
Verifying this change
I have run the benchmark and shows that this change has a minimal impact on performance.
Before this PR(I have also provided other compression provider benchmark for reference.):
After this PR:
This change added tests.
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation