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

Use awaitSingle operator instead of awaitFirst #1355

Conversation

GeorgePap-719
Copy link
Contributor

@GeorgePap-719 GeorgePap-719 commented Oct 10, 2022

A minor refactoring to use awaitSingleXxx operators instead of awaitFirstXxx.

Operators awaitFirstXxx are going to be deprecated. Also, awaitFirst operator has no value on Mono types.

  • You have read the Spring Data contribution guidelines.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 10, 2022
@GeorgePap-719 GeorgePap-719 force-pushed the kotlin-reactive-select-extensions branch from d4c3cc0 to 93a233e Compare October 10, 2022 18:24
@GeorgePap-719 GeorgePap-719 changed the title Fix consistency of extensions Fix consistency of kotlin coroutines extensions Oct 10, 2022
@GeorgePap-719 GeorgePap-719 marked this pull request as ready for review October 11, 2022 05:34
@GeorgePap-719 GeorgePap-719 force-pushed the kotlin-reactive-select-extensions branch from 93a233e to 5f3d4fa Compare October 11, 2022 05:39
@mp911de
Copy link
Member

mp911de commented Oct 11, 2022

awaitFirst() is going to be deprecated so we rather need to switch to awaitSingle() methods.

See

https://github.com/Kotlin/kotlinx.coroutines/blob/550cdf6ebf2fe4d226f057fbbf3745fe92c6e722/reactive/kotlinx-coroutines-reactor/src/Mono.kt#L163

Even though the end-result is the same,

In fact, there's (or has been) a subtle change. awaitFirst lets the coroutine continue even when the stream hasn't finished yet. This can lead easily to transactional visibility issues whereas awaitSingle() continues only when the operation has completed.

That being said, we should move to awaitSingle… and friends.

@GeorgePap-719
Copy link
Contributor Author

Oh ok, mb did't see the were deprecated. Should i open a different mr for using awaitSingle or should i just edit this mr?

This can lead easily to transactional visibility issues

Also, maybe can you point me to a direction to understand a bit better the potential issue? (Although i kinda get the general concept).

@mp911de
Copy link
Member

mp911de commented Oct 11, 2022

Feel free to force-push your changes against this pull request.

You can find a bug report regarding the problem that arises from too early continuation at spring-projects/spring-framework#25007

Operators `awaitFirstXxx` are going to be deprecated. Also, awaitFirst operator has no value on Mono types.

Related #25007

Signed-off-by: George Papadopoulos <george.719pap@gmail.com>
@GeorgePap-719 GeorgePap-719 force-pushed the kotlin-reactive-select-extensions branch from 5f3d4fa to 168b145 Compare October 12, 2022 06:03
@GeorgePap-719 GeorgePap-719 changed the title Fix consistency of kotlin coroutines extensions Use awaitSingle operator instead of awaitFirst Oct 12, 2022
@GeorgePap-719
Copy link
Contributor Author

The usage of package kotlinx.coroutines.reactive.awaitSingle instead of the reactor's is intentional?, if it's not probably i can also change it in this mr.

@mp911de
Copy link
Member

mp911de commented Oct 12, 2022

It is intentional to stick with Publisher semantics and not with Mono semantics.

@GeorgePap-719
Copy link
Contributor Author

Ok, then i think the mr is ok from my side (unless you have some pointers ofc).

If i may ask how come we want to stick with Publisher semantics in this case (any directions like above would be welcomed)?

@mp911de
Copy link
Member

mp911de commented Oct 12, 2022

Some Mono operators send a completion signal on their own upon seeing an element without that the upstream Publisher has sent a completion signal. While that's useful in some cases, for the general flow this creates rather concurrency issues.

@mp911de
Copy link
Member

mp911de commented Oct 12, 2022

Paging @sdeleuze AwaitKts Publisher<T>.awaitSingleOrNull() is deprecated but it is required for making spring-projects/spring-framework#25007 work. Can you provide guidance?

@GeorgePap-719
Copy link
Contributor Author

Some Mono operators send a completion signal on their own upon seeing an element without that the upstream Publisher has sent a completion signal.

I guess this applies in cases such as first() ?

@sdeleuze
Copy link
Contributor

@mp911de I am going to ask to Kotlin team.

@sdeleuze
Copy link
Contributor

@mp911de So after discussion it looks like they don't plan to change those deprecations on Publisher side but Mono.awaitSingleOrNull current behavior is wrong and they plan to fix it via Kotlin/kotlinx.coroutines#3487. So once that fixed I think the idea would be to use the Mono extension. I think that would cover spring-projects/spring-framework#25007 use case as well. Do you agree?

@mp911de
Copy link
Member

mp911de commented Oct 19, 2022

Thanks Sebastien. I fully agree with the bugfix as the linked ticket exactly describes the problem and we're happy to switch to Mono extensions once the issue is fixed.

@mp911de mp911de added type: bug A general bug status: blocked An issue that's blocked on an external project change and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 19, 2022
@mp911de mp911de self-assigned this Mar 15, 2023
@mp911de mp911de removed the status: blocked An issue that's blocked on an external project change label Mar 15, 2023
@mp911de mp911de closed this in ac80e51 May 26, 2023
@mp911de mp911de added this to the 3.2 M1 (2023.1.0) milestone May 26, 2023
@mp911de mp911de changed the title Use awaitSingle operator instead of awaitFirst Use awaitSingle operator instead of awaitFirst May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants