Skip to content

Conversation

@asiliuk
Copy link

@asiliuk asiliuk commented Mar 26, 2021

Description

This leak can happen in following situation:

  • Coordinator leaves longer then presented Flow, for example as let on not yet migrated ViewModel who is presenting already migrated to Flow components
  • You present modally flow whose root is UINavigationController
  • It returns .one() with child Presentable and puts its ViewController inside root.viewControllers
  • User swipes to dismiss whole modally presented UINavigationViewController

What is happening in that case:

  1. Both UINavigationController (Flow implementing class) and child Presentable receive didDisappear

  2. Because of implementation of rx.dismissed on UIViewController event on child presentable is filtered

.filter { [weak base] _ in base?.isBeingDismissed ?? true }

And isBeingDismissed of a child is false for some reason

  1. In func coordinate(...) of FlowCoordinator there's this code
.flatMap { [weak self] in
    self?.steps(from: $0, within: flow, allowStepWhenDismissed: allowStepWhenDismissed) ?? Signal.empty()
}

Since rxDimiss of child Presentable is filtered out this code is equivalent of

.flatMap { [weak self] _ in
    .never()
}
  1. So first part of pipe in func coordinate() is destroyed because Flow rxDismissed emitted event
  2. Bottom part leaves forever and is disposed only when FlowCoordinator with its disposeBag is destroyed
  3. And because FlowCoordinator in some cases can leave longer that Flow that it presents it can cause unexpected leaks

Basically fix is to destroy whole pipe when presented Flow is dimissed

Checklist

  • this PR is based on develop or a 'develop related' branch
  • the commits inside this PR have explicit commit messages
  • the Jazzy documentation has been generated (if needed -> Jazzy RxFlow)

@rxswiftcommunity
Copy link

rxswiftcommunity bot commented Mar 26, 2021

Thanks a lot for contributing @asiliuk! 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

@asiliuk
Copy link
Author

asiliuk commented Apr 7, 2021

@twittemb Hi! Do you have any thoughts about this?

@benjohnde benjohnde requested a review from twittemb April 7, 2021 11:03
@twittemb
Copy link
Collaborator

twittemb commented Apr 8, 2021

Sorry guys, I’ll review it ASAP I promise (before the end of the week).

@twittemb twittemb self-assigned this Apr 11, 2021
@twittemb twittemb changed the title Fix Flow leak when has .one() step and is only Flow emits rxDismissed Fix Flow leak when has .one() step and is only Flow emits rxDismissed #trivial Apr 11, 2021
@twittemb twittemb changed the base branch from main to develop April 11, 2021 20:20
@twittemb
Copy link
Collaborator

Thanks a lot

@twittemb twittemb merged commit 326f549 into RxSwiftCommunity:develop Apr 11, 2021
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.

2 participants