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

First observer value not being fired using the queue #32

Closed
Edudjr opened this issue Dec 17, 2019 · 5 comments
Closed

First observer value not being fired using the queue #32

Edudjr opened this issue Dec 17, 2019 · 5 comments

Comments

@Edudjr
Copy link
Contributor

Edudjr commented Dec 17, 2019

I have a scenario where I'm using an observer in a singleton, so it's shared between some classes. The problem arises when this observer triggers the instantiation of another class which also uses the same observable (It looks like this is related to #19 ). In this scenario, the app freezes because since the first event is not using the queue, the second time I subscribe it reaches lock for the second time, before the first has a chance to unlock.

I noticed that in line 45, the queue is not really used at all.

Is there any reason for that? Or do you think we could use the queue on that line, like this:

   if let queue = queue {
       queue.async {
           observer(self.value, nil)
       }
   } else {
       observer(value, nil)
   }

Here are some test scenarios comparing the old/new implementation:

    // Hangs with current implementation
    // Succeeds with new implementation
    func test_whenChangingValue_shouldSucceed() {
        let exp = expectation(description: "")
        let observable = Observable(0)

        observable.observe(DispatchQueue.main) { _, _ in
            observable.removeAllObservers()
            observable.value = 1
            exp.fulfill()
        }.add(to: &disposal)

        wait(for: [exp], timeout: 1.0)
        XCTAssert(true)
    }

    // Succeeds with current implementation
    // Succeeds with new implementation
    func test_whenUsingDispatchMain_shouldSucceed() {
        let exp = expectation(description: "")
        let singleton = Singleton.shared
        let callback: (() -> Void) = {
            exp.fulfill()
        }
        var aClass: AClass?

        singleton.observable.observe { _, _ in
            singleton.observable.removeAllObservers()
            DispatchQueue.main.async {
                aClass = AClass(callback: callback)
            }
        }.add(to: &disposal)

        print(aClass.debugDescription)
        wait(for: [exp], timeout: 1.0)
        XCTAssert(true)
    }

    // Hangs with current implementation
    // Succeeds with new implementation
    func test_whenPassingDispatchMain_shouldSucceed() {
        let exp = expectation(description: "")
        let singleton = Singleton.shared
        let callback: (() -> Void) = {
            exp.fulfill()
        }
        var aClass: AClass?

        singleton.observable.observe(DispatchQueue.main) { _, _ in
            singleton.observable.removeAllObservers()
            aClass = AClass(callback: callback)
        }.add(to: &disposal)

        print(aClass.debugDescription)
        wait(for: [exp], timeout: 2.0)
        XCTAssert(true)
    }

    // Hangs with current implementation
    // Succeeds with new implementation
    func test_whenUsingDispatchGlobal_shouldSucceed() {
        let exp = expectation(description: "")
        let singleton = Singleton.shared
        let callback: (() -> Void) = {
            exp.fulfill()
        }
        var aClass: AClass?

        singleton.observable.observe(DispatchQueue.global()) { _, _ in
            singleton.observable.removeAllObservers()
            aClass = AClass(callback: callback)
        }.add(to: &disposal)

        print(aClass.debugDescription)
        wait(for: [exp], timeout: 1.0)
        XCTAssert(true)
    }

    // Hangs with current implementation
    // Hangs with new implementation
    func test_whenNotUsingDispatch_shouldHang() {
        let exp = expectation(description: "")
        let singleton = Singleton.shared
        let callback: (() -> Void) = {
            exp.fulfill()
        }
        var aClass: AClass?

        singleton.observable.observe { _, _ in
            singleton.observable.removeAllObservers()
            aClass = AClass(callback: callback)
        }.add(to: &disposal)

        print(aClass.debugDescription)
        wait(for: [exp], timeout: 1.0)
        XCTAssert(true)
    }

    class AClass {
        var disposal = Disposal()

        init(callback: @escaping () -> Void) {
            let singleton = Singleton.shared
            singleton.observable.observe { _, _ in
                callback()
            }.add(to: &disposal)
        }
    }

    class Singleton {
        static let shared = Singleton()
        var observable = Observable(0)
        private init() {}
    }
@4brunu
Copy link
Contributor

4brunu commented Jan 6, 2020

Hey, I think you are right.
If you can, it would be nice to have that fix in a PR 👍

@Edudjr
Copy link
Contributor Author

Edudjr commented Jan 6, 2020

@4brunu sure :) should I also bump the pod version?

@4brunu
Copy link
Contributor

4brunu commented Jan 6, 2020

I'm not the owner of this library, I just would like to see this merge 🙂
But I would say, let the owner bump the version

@roberthein
Copy link
Owner

Yup, I can bump the version! 🙏

@roberthein
Copy link
Owner

Thank you @Edudjr and @4brunu, I just published Observable (2.0.0) successfully. 🙏

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

No branches or pull requests

3 participants