surfstudio / ReactiveDataDisplayManager

MIT License
34 stars 13 forks source link

CollectionPluginsChecker async AssertionFailure #255

Closed dmitryd20 closed 10 months ago

dmitryd20 commented 11 months ago

Short version of CollectionPluginsChecker looks like this:


final class CollectionPluginsChecker {

    weak var delegate: CollectionDelegate?

...
    func asyncCheckPlugins() {
        DispatchQueue.main.async {
            self.checkPlugins()
        }
    }

...
    func checkPlugin(for generator: SelectableItem?) {
        let plugin = delegate?.collectionPlugins.plugins.first(where: { $0 is CollectionSelectablePlugin })

...
        guard generator != nil && plugin == nil && eventsNotEmpty else { return }
        assertionFailure("❗️Include the CollectionSelectablePlugin.")
    }

}

CollectionPluginsChecker has weak reference to CollectionDelegate instance, so it is possible that CollectionDelegate is already deallocated when checkPlugin() is being called, which leads to an unexpected AssertionFailure. This situation can occur when CollectionManager is removed right after calling ddm.forceRefill(). checkPlugins inside forceRefill is called asynchronously and CollectionManager with its delegate are deallocated before one of checkPlugin calls.

The described case can be easily fixed with a strong reference to the delegate here.

NullIsOne commented 11 months ago

Good catch. But I think that solution can be different.

What if we add one more guard-block to unwrap delegate but not fire assertion? It should work also.

dmitryd20 commented 11 months ago

What if we add one more guard-block to unwrap delegate but not fire assertion? It should work also.

Sure, your solution looks better

NullIsOne commented 11 months ago

Merged and released in 7.3.9 (SPM - already, Cocoapods - in progress)