[FIXED] RACE in Sublist #641
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This would manifest for instance when server tries to send messages to queue subscribers and a subscription is unsubsribed at the same time. Resolves #640
|
@derekcollison Not sure if this the only way to fix, but clearly there was an issue between the results from a match and a subscription being removed from the sublist. |
server/sublist.go
Outdated
| // Store in cache and return to caller a copy of the results to avoid | ||
| // race when sub is removed from sublist and caller walks through the | ||
| // results. | ||
| cr := copyResult(result) |
There was a problem hiding this comment.
This feels like it could be a performance hit for sure.
There was a problem hiding this comment.
I revised to just copy where needed (in matchLevel)
| @@ -1,3 +1,6 @@ | |||
| // Copyright 2012-2017 Apcera Inc. All rights reserved. | |||
| // Copyright 2018 Synadia Communications Inc. All rights reserved. | |||
There was a problem hiding this comment.
I will change all of these at some point next week.
| for _, qsub := range r.qsubs { | ||
| for i := 0; i < len(qsub); i++ { | ||
| sub := qsub[i] | ||
| if string(sub.queue) != "bar" { |
There was a problem hiding this comment.
Should we rewrite it above to make it deterministic?
There was a problem hiding this comment.
Not sure what you mean here?
…ults Instead of making a copy of the whole results, make sure that we don't pass a sublist array to the result but its copy.
server/sublist.go
Outdated
| results.qsubs = append(results.qsubs, qr) | ||
| copyqr := make([]*subscription, len(qr)) | ||
| copy(copyqr, qr) | ||
| results.qsubs = append(results.qsubs, copyqr) |
There was a problem hiding this comment.
Any noticeable performance hit? Any other way to avoid the allocation and copy even of the array?
Use the append([]*subscription(nil), qr...) notation instead of make()+copy().
kozlovic
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This would manifest for instance when server tries to send messages
to queue subscribers and a subscription is unsubsribed at the same
time.
Resolves #640