Skip to content

Commit

Permalink
Fixed a race condition in the Signal.observe.
Browse files Browse the repository at this point in the history
An explicit guard is added to ensure the old signal state snapshot does not deinitialize before `updateLock` is released. Otherwise, it might result in a deadlock in cases where a `Signal` legitimately receives terminal events recursively as a result of the deinitialization of the snapshot.
  • Loading branch information
andersio committed Feb 17, 2017
1 parent 7920f19 commit 8604418
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 6 deletions.
2 changes: 1 addition & 1 deletion Carthage/Checkouts/Nimble
Submodule Nimble updated 77 files
+36 −0 .github/ISSUE_TEMPLATE
+14 −0 .github/PULL_REQUEST_TEMPLATE
+2 −0 .hound.yml
+16 −0 .swiftlint.yml
+1 −1 LICENSE
+2 −2 Nimble.podspec
+74 −5 Nimble.xcodeproj/project.pbxproj
+105 −2 README.md
+9 −9 Sources/Lib/CwlPreconditionTesting/CwlPreconditionTesting/CwlBadInstructionException.swift
+20 −21 Sources/Lib/CwlPreconditionTesting/CwlPreconditionTesting/CwlCatchBadInstruction.swift
+3 −3 Sources/Lib/CwlPreconditionTesting/CwlPreconditionTesting/CwlCatchBadInstructionPOSIX.swift
+1 −1 Sources/Lib/CwlPreconditionTesting/CwlPreconditionTesting/CwlDarwinDefinitions.swift
+0 −1 Sources/Nimble/Adapters/AssertionDispatcher.swift
+2 −2 Sources/Nimble/Adapters/AssertionRecorder.swift
+3 −3 Sources/Nimble/Adapters/NMBExpectation.swift
+1 −1 Sources/Nimble/Adapters/NMBObjCMatcher.swift
+7 −8 Sources/Nimble/Adapters/NimbleXCTestHandler.swift
+3 −3 Sources/Nimble/DSL+Wait.swift
+5 −10 Sources/Nimble/Expectation.swift
+1 −1 Sources/Nimble/Expression.swift
+1 −1 Sources/Nimble/FailureMessage.swift
+14 −20 Sources/Nimble/Matchers/AllPass.swift
+7 −12 Sources/Nimble/Matchers/AsyncMatcherWrapper.swift
+18 −8 Sources/Nimble/Matchers/BeAKindOf.swift
+15 −5 Sources/Nimble/Matchers/BeAnInstanceOf.swift
+2 −2 Sources/Nimble/Matchers/BeCloseTo.swift
+0 −1 Sources/Nimble/Matchers/BeEmpty.swift
+1 −2 Sources/Nimble/Matchers/BeGreaterThan.swift
+2 −3 Sources/Nimble/Matchers/BeIdenticalTo.swift
+1 −1 Sources/Nimble/Matchers/BeLessThan.swift
+3 −3 Sources/Nimble/Matchers/BeVoid.swift
+1 −3 Sources/Nimble/Matchers/BeginWith.swift
+2 −4 Sources/Nimble/Matchers/Contain.swift
+59 −0 Sources/Nimble/Matchers/ContainElementSatisfying.swift
+2 −5 Sources/Nimble/Matchers/EndWith.swift
+5 −5 Sources/Nimble/Matchers/Equal.swift
+1 −1 Sources/Nimble/Matchers/Match.swift
+2 −2 Sources/Nimble/Matchers/MatcherProtocols.swift
+2 −2 Sources/Nimble/Matchers/PostNotification.swift
+2 −2 Sources/Nimble/Matchers/RaisesException.swift
+10 −13 Sources/Nimble/Matchers/SatisfyAnyOf.swift
+5 −5 Sources/Nimble/Matchers/ThrowAssertion.swift
+2 −2 Sources/Nimble/Matchers/ThrowError.swift
+3 −3 Sources/Nimble/Utils/Async.swift
+1 −2 Sources/Nimble/Utils/Errors.swift
+4 −4 Sources/Nimble/Utils/SourceLocation.swift
+5 −6 Sources/Nimble/Utils/Stringers.swift
+4 −0 Sources/NimbleObjectiveC/DSL.h
+4 −0 Sources/NimbleObjectiveC/DSL.m
+1 −1 Sources/NimbleObjectiveC/NMBExceptionCapture.h
+1 −1 Tests/LinuxMain.swift
+6 −6 Tests/NimbleTests/AsynchronousTest.swift
+1 −1 Tests/NimbleTests/Helpers/XCTestCaseProvider.swift
+1 −1 Tests/NimbleTests/Helpers/utils.swift
+15 −15 Tests/NimbleTests/Matchers/AllPassTest.swift
+56 −23 Tests/NimbleTests/Matchers/BeAKindOfTest.swift
+37 −16 Tests/NimbleTests/Matchers/BeAnInstanceOfTest.swift
+8 −8 Tests/NimbleTests/Matchers/BeCloseToTest.swift
+5 −5 Tests/NimbleTests/Matchers/BeEmptyTest.swift
+1 −1 Tests/NimbleTests/Matchers/BeGreaterThanTest.swift
+4 −4 Tests/NimbleTests/Matchers/BeIdenticalToObjectTest.swift
+1 −1 Tests/NimbleTests/Matchers/BeIdenticalToTest.swift
+7 −7 Tests/NimbleTests/Matchers/BeLogicalTest.swift
+1 −1 Tests/NimbleTests/Matchers/BeNilTest.swift
+86 −0 Tests/NimbleTests/Matchers/ContainElementSatisfyingTest.swift
+1 −1 Tests/NimbleTests/Matchers/ContainTest.swift
+55 −55 Tests/NimbleTests/Matchers/EqualTest.swift
+1 −1 Tests/NimbleTests/Matchers/HaveCountTest.swift
+4 −4 Tests/NimbleTests/Matchers/MatchTest.swift
+1 −1 Tests/NimbleTests/Matchers/RaisesExceptionTest.swift
+4 −4 Tests/NimbleTests/Matchers/SatisfyAnyOfTest.swift
+1 −1 Tests/NimbleTests/Matchers/ThrowAssertionTest.swift
+8 −8 Tests/NimbleTests/Matchers/ThrowErrorTest.swift
+12 −13 Tests/NimbleTests/SynchronousTests.swift
+9 −9 Tests/NimbleTests/UserDescriptionTest.swift
+64 −0 Tests/NimbleTests/objc/ObjCContainElementSatisfying.m
+1 −1 Tests/NimbleTests/objc/ObjCRaiseExceptionTest.m
2 changes: 1 addition & 1 deletion Carthage/Checkouts/Quick
Submodule Quick updated 52 files
+29 −9 .Package.test.swift
+0 −3 .swiftlint.yml
+4 −0 .travis.yml
+53 −0 Dangerfile
+3 −0 Documentation/en-us/SettingUpYourXcodeProject.md
+2 −2 Documentation/en-us/SharedExamples.md
+116 −0 Documentation/pt-br/SharedExamples.md
+1 −1 Externals/Nimble
+2 −0 Gemfile
+72 −26 Gemfile.lock
+28 −8 Package.swift
+1 −1 Quick Templates/Quick Configuration Class.xctemplate/Swift/___FILEBASENAME___.swift
+1 −1 Quick.podspec
+100 −92 Quick.xcodeproj/project.pbxproj
+6 −6 Quick.xcodeproj/xcshareddata/xcschemes/Quick-iOS.xcscheme
+6 −6 Quick.xcodeproj/xcshareddata/xcschemes/Quick-macOS.xcscheme
+6 −6 Quick.xcodeproj/xcshareddata/xcschemes/Quick-tvOS.xcscheme
+14 −11 README.md
+8 −6 Sources/Quick/Callsite.swift
+1 −1 Sources/Quick/Configuration/Configuration.swift
+32 −1 Sources/Quick/Configuration/QuickConfiguration.swift
+26 −12 Sources/Quick/DSL/DSL.swift
+17 −11 Sources/Quick/DSL/World+DSL.swift
+10 −8 Sources/Quick/Example.swift
+5 −5 Sources/Quick/ExampleGroup.swift
+3 −3 Sources/Quick/Hooks/Closures.swift
+2 −2 Sources/Quick/Hooks/ExampleHooks.swift
+9 −0 Sources/Quick/NSBundle+CurrentTestBundle.swift
+33 −0 Sources/Quick/NSString+C99ExtendedIdentifier.swift
+11 −6 Sources/Quick/QuickMain.swift
+1 −2 Sources/Quick/QuickSelectedTestSuiteBuilder.swift
+77 −3 Sources/Quick/QuickSpec.swift
+5 −4 Sources/Quick/World.swift
+0 −17 Sources/QuickObjectiveC/NSString+QCKSelectorName.h
+0 −37 Sources/QuickObjectiveC/NSString+QCKSelectorName.m
+9 −38 Sources/QuickObjectiveC/QuickSpec.m
+55 −0 Sources/QuickSpecBase/QuickSpecBase.m
+11 −0 Sources/QuickSpecBase/include/QuickSpecBase.h
+4 −4 Tests/LinuxMain.swift
+12 −6 Tests/QuickTests/QuickFocusedTests/FocusedTests.swift
+2 −2 Tests/QuickTests/QuickTestHelpers/XCTestCaseProvider.swift
+3 −3 Tests/QuickTests/QuickTests/FunctionalTests/AfterEachTests.swift
+2 −2 Tests/QuickTests/QuickTests/FunctionalTests/BeforeEachTests.swift
+1 −1 Tests/QuickTests/QuickTests/FunctionalTests/BeforeSuiteTests.swift
+30 −0 Tests/QuickTests/QuickTests/FunctionalTests/BundleModuleNameTests.swift
+1 −1 Tests/QuickTests/QuickTests/FunctionalTests/Configuration/AfterEach/Configuration+AfterEachTests.swift
+1 −1 Tests/QuickTests/QuickTests/FunctionalTests/Configuration/BeforeEach/Configuration+BeforeEachTests.swift
+1 −1 Tests/QuickTests/QuickTests/FunctionalTests/DescribeTests.swift
+1 −1 Tests/QuickTests/QuickTests/FunctionalTests/ItTests.swift
+1 −1 Tests/QuickTests/QuickTests/FunctionalTests/PendingTests.swift
+1 −1 Tests/QuickTests/QuickTests/FunctionalTests/SharedExamples+BeforeEachTests.swift
+1 −1 Tests/QuickTests/QuickTests/FunctionalTests/SharedExamplesTests.swift
17 changes: 14 additions & 3 deletions Sources/Signal.swift
Original file line number Diff line number Diff line change
Expand Up @@ -313,13 +313,24 @@ public final class Signal<Value, Error: Swift.Error> {
return ActionDisposable { [weak self] in
if let s = self {
s.updateLock.lock()

if case let .alive(snapshot) = s.state {
var observers = snapshot.observers
observers.remove(using: token)
s.state = .alive(AliveState(observers: observers,
retaining: observers.isEmpty ? nil : self))

// Ensure the old signal state snapshot does not deinitialize before
// `updateLock` is released. Otherwise, it might result in a
// deadlock in cases where a `Signal` legitimately receives terminal
// events recursively as a result of the deinitialization of the
// snapshot.
withExtendedLifetime(snapshot) {
s.state = .alive(AliveState(observers: observers,
retaining: observers.isEmpty ? nil : self))
s.updateLock.unlock()
}
} else {
s.updateLock.unlock()
}
s.updateLock.unlock()
}
}
} else {
Expand Down
17 changes: 16 additions & 1 deletion Tests/ReactiveSwiftTests/ActionSpec.swift
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ class ActionSpec: QuickSpec {
action.errors.observeValues { errors.append($0) }
action.completed.observeValues { completedCount += 1 }
}

it("should retain the state property") {
var property: MutableProperty<Bool>? = MutableProperty(false)
weak var weakProperty = property
Expand Down Expand Up @@ -114,6 +114,21 @@ class ActionSpec: QuickSpec {
expect(action.isExecuting.value) == false
}

it("should not deadlock") {
final class ViewModel {
let action2 = Action<(), (), NoError> { SignalProducer(value: ()) }
}

let action1 = Action<(), ViewModel, NoError> { SignalProducer(value: ViewModel()) }

action1.values
.flatMap(.latest) { viewModel in viewModel.action2.values.map { _ in viewModel } }
.observeValues { _ in }

action1.apply().start()
action1.apply().start()
}

describe("completed") {
beforeEach {
enabled.value = true
Expand Down

0 comments on commit 8604418

Please sign in to comment.