Skip to content
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

[FIXED] JetStream: data race between consumer and stream when updated #5820

Merged
merged 3 commits into from
Aug 23, 2024

Conversation

kozlovic
Copy link
Member

Some tests were showing often a race between updateWithAdvisory() and isFiltered().

There may be other data races in other files that read the mset's configuration without proper locking, but this PR addresses specifically issues between consumer (consumer.go) and stream.

Signed-off-by: Ivan Kozlovic [email protected]

Some tests were showing often a race between updateWithAdvisory()
and isFiltered().

There may be other data races in other files that read the mset's
configuration without proper locking, but this PR addresses
specifically issues between consumer (consumer.go) and stream.

Signed-off-by: Ivan Kozlovic <[email protected]>
@kozlovic kozlovic requested a review from a team as a code owner August 23, 2024 03:20
@kozlovic kozlovic requested a review from derekcollison August 23, 2024 03:20
Copy link
Member Author

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

@derekcollison I think I don't even need to call to mset.getCfg() since we are protected by the stream's lock there. If you agree, I will update the PR tomorrow.

Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

@derekcollison
Copy link
Member

I will merge after all green.

@kozlovic
Copy link
Member Author

@derekcollison I got a data race on a test for completely different thing (non JS). Can I add it here or do you prefer new PR for that?

Signed-off-by: Ivan Kozlovic <[email protected]>
@kozlovic
Copy link
Member Author

Sorry, I did not wait for your answer. It was a quick fix, so added to this PR.

@kozlovic
Copy link
Member Author

@derekcollison Ok, got all green, ready to merge!

@derekcollison derekcollison merged commit f913c39 into main Aug 23, 2024
5 checks passed
@derekcollison derekcollison deleted the fix_mset_consumer_data_race branch August 23, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants