Skip to content

Conversation

@simba909
Copy link
Contributor

@simba909 simba909 commented May 16, 2021

I've recently been building a feature where I'm setting up a publisher using Publishers.Create on the main thread, and then sending values to the generated subscriber from another thread (this might an unsupported use-case for this publisher, let me know if so!). When debugging the code with Thread Sanitizer I get a warning about an access race:

SUMMARY: ThreadSanitizer: Swift access race (MyProject:arm64+0x1014d7318) in DemandBuffer.buffer(value:)+0x7c0

Looking at the code I can see that a NSRecursiveLock is used to protect the demandState inside of the DemandBuffer, but only in the flush() function. Since the demandState property is also accessed from within the buffer(value:) function my hunch is that this too should be protected by the lock. The use of the lock here leads me to believe that my threading case might be valid, but again let me know if this isn't the case.

This PR adds locking to the buffer(value:) function. Tests all seem to pass, and Thread Sanitizer is no longer upset with my code 👍🏻

This fixes thread race issues when (for example) a subscriber created by `AnyPublisher.Create` is accessed outside of the thread in which it was created.
Copy link
Member

@freak4pc freak4pc left a comment

Choose a reason for hiding this comment

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

Looks like a good fix to me. Thanks for catching it!
Can you fix the one minor formatting thing here?

Co-authored-by: Shai Mishali <freak4pc@gmail.com>
@simba909
Copy link
Contributor Author

@freak4pc Thank you for the review! Committed your suggested formatting change 👍🏻

@codecov
Copy link

codecov bot commented May 18, 2021

Codecov Report

Merging #90 (60104fa) into main (730d272) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head 60104fa differs from pull request most recent head 25958f0. Consider uploading reports for the commit 25958f0 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main      #90      +/-   ##
==========================================
- Coverage   97.12%   97.06%   -0.06%     
==========================================
  Files          62       62              
  Lines        3336     3338       +2     
==========================================
  Hits         3240     3240              
- Misses         96       98       +2     
Impacted Files Coverage Δ
Sources/Common/DemandBuffer.swift 100.00% <100.00%> (ø)
Sources/Operators/Internal/Timer.swift 67.76% <0.00%> (-1.32%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 730d272...25958f0. Read the comment docs.

@freak4pc freak4pc merged commit 60a9c36 into CombineCommunity:main May 18, 2021
kabiroberai added a commit to kabiroberai/swift-composable-architecture that referenced this pull request Nov 1, 2024
stephencelis added a commit to pointfreeco/swift-composable-architecture that referenced this pull request Nov 12, 2024
See also: CombineCommunity/CombineExt#90

Co-authored-by: Stephen Celis <stephen@stephencelis.com>
ChloeMayhewELT pushed a commit to ChloeMayhewELT/swift-composable-architecture that referenced this pull request Jan 8, 2025
See also: CombineCommunity/CombineExt#90

Co-authored-by: Stephen Celis <stephen@stephencelis.com>
# Conflicts:
#	Sources/ComposableArchitecture/Internal/Create.swift
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