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 a deadlock upon disposal in AggregateBuilder. #471

Merged
merged 2 commits into from
Jul 5, 2017

Conversation

andersio
Copy link
Member

@andersio andersio commented Jun 30, 2017

Fixes #470.
Removed also a stale test case.

The deadlock happens when a value event of a combined signal triggers termination of one of its source signals (except for the one that indirectly yields the value event).

Example: https://github.com/ReactiveCocoa/ReactiveSwift/pull/471/files#diff-8c46f2726f3ee5a6dbf7ad24b29dbd57R2795

The value from B yields a new (A, B) tuple, leading to termination of A and eventually informing the strategy with the completion of A. Since the delivery of the tuple is in progress, the strategy's internal lock is still acquired, resulting in the deadlock.

Threading of the signal aggregate strategies now mirrors that of Signal.Core to allow recursive termial event sent by a value event.

Checklist

  • Updated CHANGELOG.md.

@andersio andersio added the bug label Jun 30, 2017
@andersio andersio force-pushed the aggregate-builder-deadlock-fix branch from 3b82d2c to 3c6d88a Compare June 30, 2017 07:32
@andersio andersio requested a review from mdiep June 30, 2017 09:18
@andersio andersio force-pushed the aggregate-builder-deadlock-fix branch 5 times, most recently from 98eb89d to 3b1bf4f Compare July 2, 2017 10:28
@andersio andersio force-pushed the aggregate-builder-deadlock-fix branch from 3b1bf4f to b82ea4f Compare July 3, 2017 17:55
@mdiep mdiep merged commit aace40f into master Jul 5, 2017
@mdiep mdiep deleted the aggregate-builder-deadlock-fix branch July 5, 2017 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants