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

Flow firstOrNull support #1796

Open
wants to merge 1 commit into
base: develop
from

Conversation

@bradynpoulsen
Copy link

bradynpoulsen commented Feb 11, 2020

Adds implementations for Flow:

  • suspend fun firstOrNull(): T?
  • suspend fun firstOrNull(predicate: suspend (T) -> Boolean): T?

It is not clear why they were intentionally left out in the original proposal (#1078) but introduced with single/singleOrNull

@bradynpoulsen bradynpoulsen changed the base branch from master to develop Feb 11, 2020
@bradynpoulsen bradynpoulsen force-pushed the bradynpoulsen:feature/flow-firstOrNull branch from 6b179a1 to b8647a7 Feb 11, 2020
@bradynpoulsen bradynpoulsen force-pushed the bradynpoulsen:feature/flow-firstOrNull branch from b8647a7 to fd2ef83 Feb 11, 2020
* The terminal operator that returns the first element emitted by the flow and then cancels flow's collection.
* Returns [null] if the flow did not contain an element matching the [predicate].
*/
public suspend fun <T> Flow<T>.firstOrNull(predicate: suspend (T) -> Boolean): T? {

This comment has been minimized.

Copy link
@LouisCAD

LouisCAD Feb 11, 2020

Contributor

Could be inline, with crossinline lambda, just like most Flow operators.

This comment has been minimized.

Copy link
@bradynpoulsen

bradynpoulsen Feb 11, 2020

Author

Super odd that wasn't done with the existing first operator as well

This comment has been minimized.

Copy link
@bradynpoulsen

bradynpoulsen Feb 11, 2020

Author

Looks like I won't be able to inline this because it uses the internal NULL and AbortFlowException (which is why first isn't inlined either)

@fvasco fvasco mentioned this pull request Feb 11, 2020
@elizarov

This comment has been minimized.

Copy link
Member

elizarov commented Feb 12, 2020

What's your use-case for this function? Where does it show up?

@dave08

This comment has been minimized.

Copy link

dave08 commented Feb 12, 2020

My use case is I have a Flow that could emit 0 or 1 elements matching a certain criteria, singleOrNull { } will crash if there's 0 and first { } too.

@elizarov

This comment has been minimized.

Copy link
Member

elizarov commented Feb 12, 2020

@dave08 I'm not sure I understand. Could you just use .filter { ... }.singleOrNull() ? Are you really looking for singleOrNull { ... } (a variant with prediacate, just like available for standard collections) to cover your use-case?

@dave08

This comment has been minimized.

Copy link

dave08 commented Feb 12, 2020

That throws if there's more than one element that matches filter { } I need only the first one if there is, and if not, just null.

@LouisCAD

This comment has been minimized.

Copy link
Contributor

LouisCAD commented Feb 12, 2020

@elizarov firstOrNull returns null only if the flow completes and is empty. Otherwise, it returns the first value, and cancels the upstream flow. I currently see no combination of operators that can conveniently reproduce the behavior or firstOrNull.

@elizarov

This comment has been minimized.

Copy link
Member

elizarov commented Feb 12, 2020

@LouisCAD Indeed, that difference between firstOrNull and singleOrNull is that the former cancels the flow after it receives the first element, while the latter receives more at throws an exception if there's more than one. And that's why I'm puzzled. @dave08 said that this PR addresses a use-case of a flow that is known to have 0 or 1 elements. So why this PR is adding firstOrNull instead of singleOrNull?

@fvasco

This comment has been minimized.

Copy link
Contributor

fvasco commented Feb 12, 2020

It isn't too sexy, but it works.

suspend fun <T> Flow<T>.firstOrNull() = take(1).fold(null as T?) { _, v -> v }

A generic version

suspend fun <T> Flow<T>.getOrNull(index: Int) = take(index + 1).fold(null as T?) { _, v -> v }
suspend fun <T> Flow<T>.firstOrNull() = getOrNull(0)
@LouisCAD

This comment has been minimized.

Copy link
Contributor

LouisCAD commented Feb 12, 2020

@elizarov I understand why you're puzzled now. I think singleOrNull is a different use case, with semantics that are more complex because of implications: "single" implies assertion, but "null" implies the contrary. That's not straightforward if you add a predicate, it's no longer just 0 or 1 value or throw.

I think it is out of the scope of this PR that is for firstOrNull, which has simpler semantics (accepts any flow, never throws by itself, no kind of contradiction in the wording).

If singleOrNull { ... } is needed, people should probably open an issue about their use case first before possibly adding it if it makes sense.

@elizarov

This comment has been minimized.

Copy link
Member

elizarov commented Feb 12, 2020

I'm not just puzzled. I'm looking for use-cases. This PR proposes to add firstOrNull, but I don't have a use-case for it on the table. All I have for now is a use-case for singleOrNull.

@LouisCAD

This comment has been minimized.

Copy link
Contributor

LouisCAD commented Feb 12, 2020

@bradynpoulsen Can you give yours?

@RBusarow

This comment has been minimized.

Copy link

RBusarow commented Feb 12, 2020

I've been using a no-arg version of firstOrNull for a while now.

With Android Bluetooth, we can only set the callback once (upon connection), so all responses are returned as a stream (Flow) and we need to parse them out. Hence our use for firstOrNull().

We're writing some data over Bluetooth (discover services, write a descriptor, write a characteristic), then observing a Flow for the response. Packets can be dropped or something can happen on the peripheral's end, so there's no guarantee of actually getting that response.

In send, we suspend until we get a value or the coroutine is cancelled upstream. In the case of writing to the Characteristic, it's wrapped in a withTimeoutOrNull.

interface GattFlows : BluetoothConnectionElement {
  ...
  fun characteristicWrite(uuid: UUID): Flow<CharacteristicWrite>
  ...
}
suspend fun sendCharacteristic(
  characteristic: Characteristic,
  data: ByteArray
) = withTimeoutOrNull(TIMEOUT) {

  send(gattFlows.characteristicWrite(characteristic.uuid)) {
    characteristic.value = data
    gatt?.writeCharacteristic(characteristic)
  }
}

internal suspend inline fun <E> send(
  flow: Flow<GattEvent<E>>, crossinline writeBlock: () -> Unit = {}
): E? = withContext(executor) {

  flow.onStart { writeBlock() }
    .firstOrNull()                             // < -- Here's the use-case
    ?.let { response ->
      if (response.isSuccessful) {
        response.value
      } else null
    }
}

Coincidentally, it's the same function as the use-case as for #1758, but for a different reason.

@dave08

This comment has been minimized.

Copy link

dave08 commented Feb 12, 2020

My original use case was to listen for the first finished WorkInfo in Android WorkManager's getWorkInfosByTagLiveData().asFlow(), I don't want it to crash if by mistake there's another one following the first that is also finished, but I don't know if it will for some reason complete the flow without any finished results, I just don't need the extra check whether it's single, but I also don't want it to crash on none like first { }.

I guess my case is really just for prevention of unnecessary crashing (which might not really be a concern, but why take that chance?) when not knowing a third party api well enough... I could technically use a try/catch for that though it might be less clear that I'm only really interested in 0 or 1 emissions (even though there could be more) that match the criteria.

@LouisCAD

This comment has been minimized.

Copy link
Contributor

LouisCAD commented Feb 12, 2020

@dave08 Using first is safe in your case as the LiveData never ends, making the Flow never complete.

* The terminal operator that returns the first element emitted by the flow and then cancels flow's collection.
* Returns [null] if the flow was empty.
*/
public suspend fun <T> Flow<T>.firstOrNull(): T? {

This comment has been minimized.

Copy link
@mhernand40

mhernand40 Feb 13, 2020

Looking at singleOrNull(), it takes in a T : Any. Wouldn't we want to do the same for firstOrNull()? Otherwise, given a Flow<Foo?>, there would be ambiguity between a null emission and a completion without any emissions.

This comment has been minimized.

Copy link
@bradynpoulsen

bradynpoulsen Feb 13, 2020

Author

I was wondering about that. It would be ambiguous if you needed to distinguish between a null element and a null fallback, but it comes down to semantics on whether you actually care. If we look at the collections and sequences APIs, there is no T : Any requirement since there may be a totally valid reason you don't care how you got null.

To be consistent, I'm also fine with T : Any purely for consistency with existing Flow APIs.

This comment has been minimized.

Copy link
@elizarov

elizarov Feb 14, 2020

Member

We've discussed it today and had decided to leave xxxOrNull on Kotlin Flow as extensions to <T : Any>. Note, that this is not consistent with stdlib (where you can use Any? in similar operators) and it was made so because of pre-v1.0 reports that it is not convenient. We don't know if the same would apply to Flow, so leaving it as <T : Any> for now is the conservative decision here. If we find use-cases where it causes inconvenience it is always safe to wide the upper bound to Any? in future releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.