-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix flaky shared flow subscription #4489
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
base: fix-flaky-shared-flow-subscription
Are you sure you want to change the base?
Changes from all commits
2bc88ed
32ffab8
c6cf814
905fe0a
5f10d0b
ecaf088
0d6c9e8
797f97a
1cd0f36
fb7326b
b2f53c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,14 +109,24 @@ public abstract class ChannelFlow<T>( | |
| * For non-atomic start it is possible to observe the situation, | ||
| * where the pipeline after the [flowOn] call successfully executes (mostly, its `onCompletion`) | ||
| * handlers, while the pipeline before does not, because it was cancelled during its dispatch. | ||
| * Thus `onCompletion` and `finally` blocks won't be executed and it may lead to a different kinds of memory leaks. | ||
| * Thus `onCompletion` and `finally` blocks won't be executed, and it may lead to a different kind of memory leaks. | ||
| */ | ||
| public open fun produceImpl(scope: CoroutineScope): ReceiveChannel<T> = | ||
| scope.produce(context, produceCapacity, onBufferOverflow, start = CoroutineStart.ATOMIC, block = collectToFun) | ||
| produceImplInternal(scope, CoroutineStart.ATOMIC) | ||
|
|
||
| internal open fun produceImplInternal(scope: CoroutineScope, start: CoroutineStart): ReceiveChannel<T> = | ||
| scope.produce(context, produceCapacity, onBufferOverflow, start = start, block = collectToFun) | ||
|
|
||
| override suspend fun collect(collector: FlowCollector<T>): Unit = | ||
| coroutineScope { | ||
| collector.emitAll(produceImpl(this)) | ||
| // If upstream and collect have the same dispatcher, launch the `produce` coroutine undispatched. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously,
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something like this should fail, but it does not, so I assume this is a regression. |
||
| // This allows the collector to reliably subscribe to the flow before it starts emitting. | ||
| val current = currentCoroutineContext()[ContinuationInterceptor] | ||
| val desired = context[ContinuationInterceptor] | ||
| val start = if (desired == null || desired == current) { | ||
| CoroutineStart.UNDISPATCHED | ||
| } else CoroutineStart.ATOMIC | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please consider using a consistent code style. We have some legacy places where one branch is wrapped into braces while another is not, but it's rather confusing than intentional |
||
| collector.emitAll(produceImplInternal(this, start)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another problem:
For example, |
||
| } | ||
|
|
||
| protected open fun additionalToStringProps(): String? = null | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,9 +2,9 @@ | |
|
|
||
| package kotlinx.coroutines.flow | ||
|
|
||
| import kotlinx.coroutines.testing.* | ||
| import kotlinx.coroutines.* | ||
| import kotlinx.coroutines.channels.* | ||
| import kotlinx.coroutines.testing.* | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unrelated: L1: Do we need the suppression on Line 1? I don't quite understand how it is relevant here. |
||
| import kotlin.test.* | ||
|
|
||
| class FlowCallbackTest : TestBase() { | ||
|
|
@@ -14,24 +14,25 @@ class FlowCallbackTest : TestBase() { | |
| val flow = callbackFlow { | ||
| // ~ callback-based API | ||
| outerScope.launch(Job()) { | ||
| expect(2) | ||
| try { | ||
| expect(4) | ||
| send(1) | ||
| expectUnreached() | ||
| } catch (e: IllegalStateException) { | ||
| expect(3) | ||
| expect(5) | ||
| assertTrue(e.message!!.contains("awaitClose")) | ||
| } | ||
| finish(6) | ||
| } | ||
| expect(1) | ||
| } | ||
| try { | ||
| flow.collect() | ||
| } catch (e: IllegalStateException) { | ||
| expect(4) | ||
| expect(2) | ||
| assertTrue(e.message!!.contains("awaitClose")) | ||
| } | ||
| finish(5) | ||
| expect(3) | ||
| } | ||
|
|
||
| @Test | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably shouldn't be open, there is a single implementation of this method