Deadlock

[codyrobb]

Hi,

I am running into a deadlock issue. I’m not entirely sure why it is happening yet. Here is my setup (dumbed down as this is the only part breaking, can provide more if needed).

class ActionController {
	var delay: NSTimeInterval = 0.0
	let undoable = MutableProperty<Bool>(false)

	func start() {
		if delay > 0.0 {
			undoable.value = true
		}
		
		// More stuff
		// Eventually will commit once delay is hit
		// Can provide more if needed.
	}
	
	func undo() {
		guard undoable.value else {
			return
		}
		
		// Do some stuff
		
		/***** DEADLOCK HAPPENS ON LINE BELOW *******/
		undoable.value = false
		/********************************************/
	}
}

class MyViewModel {

	private let currentAction = MutableProperty<ActionController?>(nil)
	
	let undoEnabled = MutableProperty<Bool>(false)
	
	init() {
		undoEnabled <~ currentAction.producer
			.flatMap(.Concat) { action in
				guard let action = action else {
					return SignalProducer<Bool, NoError>(value: false)
				}
				
				return action.undoable.producer
			}
			.skipRepeats()
	}
	
	func commitAction() {
		let action = ActionController()
		action.delay = 10
		action.start()
	}
	
	func undoCurrentAction() {
		currentAction.value?.undo()
	}

	 func onActionCompleted() {
		// Method gets called when ActionController completes    
		currentAction.value = nil
	}

}

Both commitAction() and undoCurrentAction() get called from a UIViewController that the MyViewModel is powering and has subscribers to undoEnabled.

Example for testing:

        viewModel.undoEnabled.producer.startWithNext { [unowned self] enabled in
            if enabled {
                self.viewModel.undoCurrentAction()
            }
        }

The deadlock seems to be happening when calling undo and setting undoable’s value to false.

[codyrobb]

After setting a breakpoint on _NSLockError(), it looks like it is line 66 in Signal.swift causing the lock and eventual breakdown.

This is one of my first bigger stabs at RAC4 after coming from RAC2.X. I might be misusing some things. Please let me know!

[codyrobb]

Another note: I am using ReactiveCocoa 4.2.2, but this seemed to be a better place to post the issue as it appeared you guys were trying to get the Swift API oriented issues over here.

If I need to move this just let me know.

[codyrobb]

Hmmm... interesting.... The root of the problem appears to be this code:

        viewModel.undoEnabled.producer.startWithNext { [unowned self] enabled in
            if enabled {
                self.viewModel.undoCurrentAction()
            }
        }

If I do this:

viewModel.undoEnabled.producer.startWithNext { [unowned self] enabled in
    if enabled {
        // undoPressed simply calls viewModel.undoCurrentAction()
        self.navigationItem.rightBarButtonItem =
            UIBarButtonItem(title: "UNDO", style: .Plain, target: self, action: #selector(self.undoPressed))
    } else {
        self.navigationItem.rightBarButtonItem = nil
    }
}

Everything works fine. Can anyone elaborate on this?

[andersio]

It seems you are having an infinite feedback loop. undoCurrentAction is being called when undoEnabled changes, while undoEnabled is bound to accept values from action.undoable, which is being modified in undoCurrentAction.

[chuganzy]

This is just a simple example (actually leaking), but as you can see it is not infinite loop, so I feel it makes more sense to make Lock recursive.

enum Event {
    case a
    case b
}

let pipe = Signal<Event, NoError>.pipe()
pipe.output.filterMap { $0 == .a ? Event.b : nil }.observe(pipe.input)
pipe.output.observeValues { event in
    print("value: \(event)")
}
pipe.input.send(value: .a) // deadlock

What do you think? @andersio @mdiep

Related PR that makes Lock recursive #308

Especially when we use RAS with RAC, all events are pretty much UI driven, and sometimes UIKit causes this case unexpectedly.

[inamiy]

pipe.output.filterMap { $0 == .a ? Event.b : nil }.observe(pipe.input)

Nice example :)
I also think this kind of data-flow should be accepted, i.e. let's use recursive lock.

[mdiep]

I think that organization should be discouraged. FRP is all about unidirectional data flow, so I don't think it makes sense to support cycles.

[chuganzy]

Let me give you more concrete example:

class View {
    enum Event {
        case textChanged(text: String)
        case buttonPressed
    }
    private let eventIO = Signal<Event, NoError>.pipe() // or view model
    private let textField = UITextField()
    private let button = UIButton()

    private func bind() {
        textField.reactive.continuousTextValues
            .map { Event.textChanged(text: $0) }
            .observe(eventIO.input)
        button.reactive.controlEvents(.touchUpInside)
            .map { _ in Event.buttonPressed }
            .observe(eventIO.input)
    }

    func observe(action: @escaping (Event) -> Void) {
        eventIO.output.observeValues {
            action($0)
        }
    }
}

class ViewController {
    let someView = View()

    private func bind() {
        someView.observe { [weak self] event in
            switch event {
                case .textChanged(let text): 
                    // do something
                case .buttonPressed:
                    // do something that eventually invoke below
                    self?.view.endEditing(false)
            }
        }
    }
}

When it invokes self?.view.endEditing(false) (or just call textField.resignFirstResponder()) UIKit sometimes sends UITextView.textDidChangeNotification (custom keyboard is related I am assuming) so continuousTextValues sends new value, and that ends up with deadlock. This kind of things can easily happen (ex: Apple changes the event cycle) and it's too risky to make it deadlock and forcing app to crash.

I do understand pipe.output.filterMap { $0 == .a ? Event.b : nil }.observe(pipe.input) kind of cases should be discouraged though, it actually happens in UI world easily (and sometimes unexpectedly), and I do not see the reason to force them to crash while it works.

What do you think?

[inamiy]

FYI

• Queue-drain model without RecursiveLock: Recursive value delivery. #540
  • IMO interesting approach, but impl is quite complicated so I still prefer simple RecursiveLock
• RxSwift impl: Rx.swift#L89-L122
  • Uses RecursiveLock
  • fatalError with friendly message when #if DEBUG
  • Prints warning message at console when non-DEBUG

Since detecting cyclic dependency is hard in general and requires a lot of runtime test, I prefer the approach RxSwift is taking: prompt a message and use RecursiveLock.

And IMO "discouraged" only works when the problem is predictable and preventable beforehand.
But as far as I see many unexpected deadlock examples here, I would say the problem is overall "unpredictable".

Also, without reentrancy, I think many other potential operators e.g. #308 retryWhen can't be implemented in ReactiveSwift (unless explicitly using scheduler to move on to async).

[chuganzy]

@mdiep @andersio Can we start from reopening this issue? We might be able to gather more opinions.

[mdiep]

In that particular case, the pipe is an unnecessary level of indirection.

This seems like it'd work:

class View {
    enum Event {
        case textChanged(text: String)
        case buttonPressed
    }
    private let textField = UITextField()
    private let button = UIButton()

    func observe(action: @escaping (Event) -> Void) {
        textField.reactive.continuousTextValues
            .map { Event.textChanged(text: $0) }
            .observe(action)
        button.reactive.controlEvents(.touchUpInside)
            .map { _ in Event.buttonPressed }
            .observe(action)
    }
}

class ViewController {
    let someView = View()

    private func bind() {
        someView.observe { [weak self] event in
            switch event {
                case .textChanged(let text): 
                    // do something
                case .buttonPressed:
                    // do something that eventually invoke below
                    self?.view.endEditing(false)
            }
        }
    }
}

I don't think that the RxSwift behavior of not crashing but violating the contract is better.

[chuganzy]

That is right, but as commented in the code, we do have a view model that has some logics inside, so we do need this kind of pipe.

Also even if we did not use pipe, if want to do something like below ultimately, same thing will happen.

textView.reactive.continuousTextValues
  .filter { $0 == "resign" }
  .observe { [weak textView] _ in textView?.resignFirstResponder() }

I don't think that the RxSwift behavior of not crashing but violating the contract is better.

Agree, since sometimes we cannot prevent this happening..

What do you think of providing a way to switch the lock then?

[mdiep]

It should be possible to detect cycles when you're constructing the signal chain. 🤔

[chuganzy]

🤔

textView.reactive.continuousTextValues
  .filter { $0 == "resign" }
  .observe { [weak textView] _ in textView?.resignFirstResponder() }

How can you solve this? resignFirstResponder() sometimes sends new value though. I assume there is no way other than switching the scheduler..

[mdiep]

Oh, good point! 🤔

[chuganzy]

Yeah..
It's not UITextView specific, this kind of thing can easily happen as some methods have unexpected side effect sometimes..

[inamiy]

FYI here's a flipflop example that won't crash our real world:
https://gist.github.com/inamiy/bd7d82a77a4df70b3b4a367a2d014ca9

[RuiAAPeres]

Hello. 👋 Thanks for opening this issue. Due to inactivity, we will soft close the issue. If you feel that it should remain open, please let us know. 😄

[chuganzy]

Just for sharing, we have been patching RAS internally to workaround this.

[RuiAAPeres]

@chuganzy do you think you could open a PR with the fix?