pointfreeco / swift-composable-architecture

A library for building applications in a consistent and understandable way, with composition, testing, and ergonomics in mind.
https://www.pointfree.co/collections/composable-architecture
MIT License
12.54k stars 1.45k forks source link

Voice Memos Demo crashes when deleting memo while it is playing #182

Closed krawutzi closed 4 years ago

krawutzi commented 4 years ago

Describe the bug App crashes when deleting memo while playing it back.

To Reproduce


* This "forEach" reducer was combined with or run from another reducer that removed the element at this index when it handled this action. To fix this make sure that this "forEach" reducer is run before any other reducers that can move or remove elements from state. This ensures that "forEach" reducers can handle their actions for the element at the intended index.

* An in-flight effect emitted this action while state contained no element at this index. To fix this make sure that effects for this "forEach" reducer are canceled whenever elements are moved or removed from its state. If your "forEach" reducer returns any long-living effects, you should use the identifier-based "forEach", instead.

* This action was sent to the store while its state contained no element at this index. To fix this make sure that actions for this reducer can only be sent to a view store when its state contains an element at this index. In SwiftUI applications, use `ForEachStore`.: file /Users/___/github/swift-composable-architecture/Sources/ComposableArchitecture/Reducer.swift, line 228

Expected behavior The app does not crash when deleting a voice memo, even when it is currently being played. (There is no more action sent to the single memo reducer, when the bigger reducer has already removed it.)

Screenshots Not applicable.

Environment

Additional context Add any more context about the problem here.

krawutzi commented 4 years ago

I guess this is related to https://github.com/pointfreeco/swift-composable-architecture/pull/154

mbrandonw commented 4 years ago

Good find!

This is indeed as a result of some changes we've made recently to forEach, but it's really just pointing out that there is an application bug. In particular, this bullet point of the assertion failure message explains what is happening:

  • An in-flight effect emitted this action while state contained no element at this index. To fix this make sure that effects for this "forEach" reducer are canceled whenever elements are moved or removed from its state. If your "forEach" reducer returns any long-living effects, you should use the identifier-based "forEach", instead.

We have an in-flight effect when playing an audio track, it's the timer that updates the progress bar. When you remove that item the timer is not being cancelled, and so it eventually emits again, which means it tries to execute some logic on a row that no longer exists.

So this is actually a good error to have. It's pointing out a real application bug. We need to make sure to cancel the timer when we delete the voice memo. I don't have time to fix this problem right now, but while investigating I was able to make a failing test case:

https://github.com/pointfreeco/swift-composable-architecture/compare/voice-memos-crash?expand=1

This simulates what happens when you start playing a track and then delete it. Right now the test should pass as-is, but it fails because the timer effect is still in-flight, and the assert helper requires that all effects complete in order for the test to pass. Further, if you uncomment the lines at the bottom of the assertion, which simulates the timer ticking and delivering an action, you will get the crash you are seeing when running the app.

Hopefully we'll have some time in the coming days to fix, or if you (or someone else) is interested in giving it shot feel free!

mbrandonw commented 4 years ago

We've now merged a fix for this in #183.