Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#1103 Refactor Multi.toFlow() to avoid blocking - Mutiny1 #1104

Merged
merged 2 commits into from
Nov 2, 2022
Merged

Conversation

heubeck
Copy link
Collaborator

@heubeck heubeck commented Oct 30, 2022

  • Switching from channelFlow to callbackFlow brings simplification
  • Blocking issue (kotlin Multi<T>.asFlow() blocking the thread #1103) was caused by buffering of the channel that underlies the Flow, solved by explicit switching to an unlimited channel that doesn't buffer.

@@ -147,13 +148,13 @@ class MultiAsFlowTest {
delay(100)
emit()
emit()
yield()
Copy link
Collaborator Author

@heubeck heubeck Oct 30, 2022

Choose a reason for hiding this comment

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

this test became flaky, the onFailure was sometimes propagated before flow.collect retrieved the second item - forcing a context switch helped. need to observe this further on.

@heubeck heubeck linked an issue Oct 30, 2022 that may be closed by this pull request
@heubeck heubeck requested a review from jponge October 30, 2022 20:50
@heubeck heubeck changed the title #1103 Refactor Multi.toFlow() to avoid blocking #1103 Refactor Multi.toFlow() to avoid blocking - Mutiny1 Oct 30, 2022
@jponge
Copy link
Member

jponge commented Oct 31, 2022

Let's see progress on #1105

@heubeck heubeck marked this pull request as draft October 31, 2022 08:24
@heubeck heubeck marked this pull request as ready for review October 31, 2022 09:51
@jponge jponge added bug Something isn't working Kotlin Kotlin support labels Oct 31, 2022
@jponge jponge added this to the 1.8.0 milestone Oct 31, 2022
@jponge
Copy link
Member

jponge commented Oct 31, 2022

@heubeck You can sync from #1105 👍

@heubeck
Copy link
Collaborator Author

heubeck commented Oct 31, 2022

@heubeck You can sync from #1105 👍

really sure?
Every use of the toFlow needs to be re-compiled, I'm afraid there might be transient usages in libraries that get the bytecode-incompatible version by downstream dependency management and break at runtime.
I'll do it, if you say so, just mentioning.

@jponge
Copy link
Member

jponge commented Oct 31, 2022

Hum you have a point.

@jponge
Copy link
Member

jponge commented Oct 31, 2022

So the fix for 1.x could be to keep the method, and perform unlimited buffering, and update the docs.

WDYT?

@heubeck
Copy link
Collaborator Author

heubeck commented Oct 31, 2022

So the fix for 1.x could be to keep the method, and perform unlimited buffering, and update the docs.

WDYT?

convinced 😊
Will also add a statement for that thing to the mutiny 2 docs as it's not obvious.

documentation/src/main/jekyll/guides/kotlin.adoc Outdated Show resolved Hide resolved
@jponge jponge merged commit a5fdbfc into 1.x Nov 2, 2022
@jponge jponge deleted the issue/1103 branch November 2, 2022 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Kotlin Kotlin support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kotlin Multi<T>.asFlow() blocking the thread
2 participants