Actions broken in rxjava2-3.0.2

[gustavkarlsson]

Something with actions seems broken in 3.0.2 and I believe it could be this flatMap: https://github.com/beworker/knot/commit/fd78d3978e58406e5e6d673369f604012f060fab#diff-6eee9d229ac30d0f2976e4d1a66ffae4R285

My perform<Action> observables keep stacking on top of each other for each new action emitted, causing infinite streams in switchMaps to never be disposed.

Example:

perform<Action> {
    switchMap {
        Observable.never<Change>()
            .doOnLifecycle({
                // onSubscribe gets called for every new emitted action (as it should)
            }, {
                // onDispose never gets called (but it should get called for every switch)
            })
    }
}

[sergejsha]

The issue I fixed in 3.0.2 is the following:

If we declare an action handler like this

perform<Action.Load> {
    flatMapSingle<String> { loadData() }
        .map<Change> { Change.Load.Success(it) }
        .onErrorReturn { Change.Load.Failure(it) }
}

after the first error, the action observable completes and stops receiving new actions. The declaration solving the problem could be written like this.

perform<Action.Load> {
    flatMapSingle<String> { 
        loadData()
            .map<Change> { Change.Load.Success(it) }
            .onErrorReturn { Change.Load.Failure(it) }
    }
}

In 3.0.2 I wrapped the action block with flatMap. The action observable stopped to complete on error and solved the issue (here is the test) https://github.com/beworker/knot/blob/8d21f20195bce7b172886a49b945c041fdba44cc/knot3/src/test/kotlin/de/halfbit/knot3/KnotSingleActionTest.kt#L91-L128 but, of course, it started to ignore switching the map inside the block.

There are some solutions I could think of, but none of them is the perfect one.

Option 1

• Revert changes to 3.0.1
• Let knot fail with a clear error message at runtime, when an action observable completes

Pros:

• No changes to knot API

Cons:

• No compile-time enforcement for proper syntax: devs can write code which will not work as expected.

Option 2

Let actions to complete, making them re-executable. This can be achieved by introducing multiple perform-blocks, like performSwitch, performMap for each of RX types like Observable, Single, Maybe. The example above could be then re-written like this.

performMapSingle<Action.Load> {
    loadData()
        .map<Change> { Change.Load.Success(it) }
        .onErrorReturn { Change.Load.Failure(it) }
}

If the action handler should be switched, it will be written like this.

performSwitchSingle<Action.Load> {
    loadData()
        .map<Change> { Change.Load.Success(it) }
        .onErrorReturn { Change.Load.Failure(it) }
}

If we declare API like that, we can wrap the block with the corresponding RX-operator, letting the block itself to complete, without completing the main action observable.

Pros:

• The API is compile-time safe
• A bit shorter action block (5 lines vs. 7 lines)

Cons:

• API changes and the client code needs to be migrated
• Bigger variety of perform operators (6 perform-operators vs. 1 operator before)

@gustavkarlsson What would you say, buddy?

[gustavkarlsson]

I see... I was actually in a similar situation while working on krate and initially went with Option 2. But after a while I abandoned it because the API surface got too large and it felt like I was just wrapping a bunch of Rx operators (Single, Maybe, Observable + flatMap, switchMap, concatMap). It just became confusing to use while still being more limited than the single Observable approach.

So I would say option 1 is the way to go. I realize the subtle differences in error handling might trip people up, but it's just how Rx works. Trying to "save" the dev by doing unexpected things behind the scenes doesn't seem like a great option in my opinion. Best practices for Rx should apply here as it does everywhere else. Just my opinion of course.

[sergejsha]

Fair enough. I was looking at Option 1 too. I will add an assertion to the ~doOnComplete~ doOnAfterTerminate of action section, that the error becomes more obvious to developers. Documentation will also be updated.

FYI: @StefMa

[StefMa]

Thanks for the information 👍

[sergejsha]

The issue is fixed in release 3.0.3 (https://github.com/beworker/knot/releases/tag/rxjava2-3.0.3). Thanks for quick reporting!