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

Missing barrier flag in ConnectionRepository when mutating state #2987

Merged
merged 2 commits into from Jan 31, 2024

Conversation

laevandus
Copy link
Contributor

@laevandus laevandus commented Jan 30, 2024

🔗 Issue Links

Follow-up to PR 2985

🎯 Goal

Better thread-safety in ConnectionRepository

📝 Summary

Barrier flag was missing for mutating work item

🛠 Implementation

Since we use a concurrent queue then all the mutating work items must use a barrier flag for making sure other concurrently running work items do not try to read the state what is being mutated.

☑️ Contributor Checklist

  • I have signed the Stream CLA (required)
  • This change follows zero ⚠️ policy (required)
  • This change should be manually QAed
  • Changelog is updated with client-facing changes
  • New code is covered by unit tests
  • Comparison screenshots added for visual changes
  • Affected documentation updated (docusaurus, tutorial, CMS)

🎁 Meme

Provide a funny gif or image that relates to your work on this pull request. (Optional)

@laevandus laevandus requested a review from a team as a code owner January 30, 2024 09:04
@laevandus laevandus added 🐞 Bug An issue or PR related to a bug 🌐 SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK labels Jan 30, 2024
@@ -470,7 +470,7 @@ final class ConnectionRepository_Tests: XCTestCase {
XCTAssertEqual(result?.value, connectionId)
}

func test_connectionId_doesNotDeadlock() {
func test_connectionId_triggersCompletions_whenConcurrentlyCalled() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just renamed to match with naming to a similar test in PR 2986

Copy link
Member

@nuno-vieira nuno-vieira left a comment

Choose a reason for hiding this comment

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

Nice catch 👍 In the future we probably should simplify this part of the codebase

@laevandus laevandus enabled auto-merge (squash) January 31, 2024 09:49
Copy link

sonarcloud bot commented Jan 31, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

42 New issues
0 Security Hotspots
91.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@laevandus laevandus merged commit 1121a07 into develop Jan 31, 2024
15 checks passed
@laevandus laevandus deleted the bug/connection-repository-concurrency branch January 31, 2024 10:33
@testableapple testableapple mentioned this pull request Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 Bug An issue or PR related to a bug 🌐 SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants