Skip to content

Ensure running unregisterDelegate.dispose() on main thread.#19

Merged
dangthaison91 merged 2 commits into
RxSwiftCommunity:masterfrom
mongris:master
May 25, 2018
Merged

Ensure running unregisterDelegate.dispose() on main thread.#19
dangthaison91 merged 2 commits into
RxSwiftCommunity:masterfrom
mongris:master

Conversation

@mongris
Copy link
Copy Markdown
Contributor

@mongris mongris commented Apr 23, 2018

Fixed issue #18

@mongris mongris changed the title Fixed issue #18 Fixed issue #18Ensure running unregisterDelegate.dispose() on main thread. Apr 23, 2018
@mongris mongris changed the title Fixed issue #18Ensure running unregisterDelegate.dispose() on main thread. Ensure running unregisterDelegate.dispose() on main thread. Apr 23, 2018
@dangthaison91
Copy link
Copy Markdown
Collaborator

Thanks for your PR @mongris. I already used .observeOn(MainScheduler()), do you think
adding subscribeOn(MainScheduler()) will help?

@mongris
Copy link
Copy Markdown
Contributor Author

mongris commented Apr 24, 2018

@dangthaison91 Unfortunately it's not working. I have upload a simple project to repo this issue:
https://github.com/RxSwiftCommunity/RxASDataSources/files/1941081/RxASDataSourcesCrashSample.zip

Copy link
Copy Markdown
Contributor

@fumito-ito fumito-ito left a comment

Choose a reason for hiding this comment

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

@mongris sorry for delay. I checked your code and commented. Please check it.

}
}

return Disposables.create { [weak object] in
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about usingScheduledDisposable ?

let disposable = Disposable.create { /* do something */ }
ScheduledDisposable(scheduler: MainScheduler(), disposable: disposable)

unregisterDelegate.dispose()
default:
break
// ensure running unregisterDelegate.dispose() on main thread
Copy link
Copy Markdown
Contributor

@fumito-ito fumito-ito May 11, 2018

Choose a reason for hiding this comment

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

I found that following code cause #18

let subscription = self.asObservable()
                 .observeOn(MainScheduler())
                 // and more...

but following does not cause same problems without your patches.

             let subscription = self.asObservable()
                 .catchError { error in
                     bindingErrorToInterface(error)
                     return Observable.empty()
                 }
                 // source can never end, otherwise it would release the subscriber, and deallocate the data source
                 .concat(Observable.never())
                 .takeUntil(object.rx.deallocated)
                 .observeOn(MainScheduler()) // <= Move to here

Do you know why ? I want to make code more Rx-like if we can.

@mongris
Copy link
Copy Markdown
Contributor Author

mongris commented May 24, 2018

@fumito-ito Sorry for my delay.
Your patch looks more reasonable.
I don't know why need to move .observeOn(MainScheduler()) after .takeUntil(object.rx.deallocated) but it works with ScheduledDisposable(scheduler: MainScheduler(), disposable: disposable).
Can you merge your patch it into the next release?

@mongris
Copy link
Copy Markdown
Contributor Author

mongris commented May 24, 2018

@fumito-ito I found another solution:

The problem is unregisterDelegate is related with an ASNode which will deinit in background thread(ASDeallocThread). We can schedule unregisterDelegate into main scheduler since it is a Dispsoable object:

            let unregisterDelegate = ScheduledDisposable(
                scheduler: MainScheduler.instance,
                disposable: DelegateProxy.installForwardDelegate(dataSource, retainDelegate: retainDataSource, onProxyForObject: object)
            )

@fumito-ito
Copy link
Copy Markdown
Contributor

@mongris Great ! your second solution looks better than others. could you update your PR to wrap unregisterDelegate with ScheduledDisposable ?

Copy link
Copy Markdown
Collaborator

@dangthaison91 dangthaison91 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!!!

@dangthaison91
Copy link
Copy Markdown
Collaborator

Thanks @mongris and @fumito-ito

@dangthaison91 dangthaison91 merged commit afd0a51 into RxSwiftCommunity:master May 25, 2018
@rxswiftcommunity
Copy link
Copy Markdown

Thanks a lot for contributing @mongris! I've invited you to join the
RxSwiftCommunity GitHub organization – no pressure to accept! If you'd like
more information on what this means, check out our contributor guidelines
and feel free to reach out with any questions.

Generated by 🚫 dangerJS

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.

3 participants