Skip to content

Remove implicit forking in ZIO#onDone and ZIO#onDoneCause to align with onX methods #9286

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

Merged
merged 14 commits into from
Nov 21, 2024

Conversation

asr2003
Copy link
Contributor

@asr2003 asr2003 commented Nov 8, 2024

Updated onDone and onDoneCause to execute callbacks synchronously on the main fiber to align behavior with other onX methods removing implicit forking( (i.e. NOT fork).

Closes #9191
/claim #9191

Important Note about Changes to onDone and onDoneCause Behavior

In this release, we have updated the behavior of onDone and onDoneCause to address concerns raised in Issue #9191. Previously, these methods operated asynchronously by forking their callback effects, which could lead to subtle bugs and race conditions. This behavior was unique among ZIO's onX methods and diverged from typical expectations.

Key Behavior Change: Synchronous Execution

Starting in this release, onDone and onDoneCause now execute success and failure callbacks synchronously within the calling fiber. This update aligns them with other onX methods (e.g., onExit), ensuring that the effect completes fully, including any specified callbacks, before moving on. This change provides greater predictability and reliability by removing the previously implicit asynchronous behavior.

Summary and Impact

This enhancement addresses the risk identified in Issue #9191, where the unexpected forking could cause surprising, hard-to-trace bugs. As a result, users may notice different callback behaviors following this upgrade. Where necessary, alternative patterns (such as explicit forking) can be employed to replicate prior behavior. We encourage users to review any code that relies on onDone or onDoneCause and verify that the updated synchronous behavior aligns with expected outcomes.

@jdegoes
Copy link
Member

jdegoes commented Nov 8, 2024

@asr2003 These need tests to guarantee synchronous behavior for each.

@asr2003
Copy link
Contributor Author

asr2003 commented Nov 8, 2024

@jdegoes Sure! I will add it

Copy link
Contributor

@kyri-petrou kyri-petrou left a comment

Choose a reason for hiding this comment

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

@asr2003 in addition to John's comment, can you please do the following:

  1. Add scaladoc to both onDone and onDoneCause methods
  2. Write a paragraph explaining the behaviour change of onDone which we will use in release notes. This should be similar to the "Important note about ZPool changes" found in the v2.1.10 notes

@kyri-petrou
Copy link
Contributor

One last comment, I think the implementation should use onExit so that we're consistent across onX implementations

@asr2003
Copy link
Contributor Author

asr2003 commented Nov 13, 2024

@kyri-petrou @jdegoes Done with the changes!

asr2003 and others added 3 commits November 14, 2024 22:36
Co-authored-by: kyri-petrou <67301607+kyri-petrou@users.noreply.github.com>
Co-authored-by: kyri-petrou <67301607+kyri-petrou@users.noreply.github.com>
@asr2003
Copy link
Contributor Author

asr2003 commented Nov 14, 2024

@kyri-petrou There has been type error if we use OnExit directly on OnDoneCause

error] /home/runner/work/zio/zio/core/shared/src/main/scala/zio/ZIO.scala:1074:12: type mismatch;
[error]  found   : zio.ZIO[R1,E,A]
[error]  required: zio.ZIO[R1,Nothing,Unit]
[error]     onExit {
[error]            ^

So I have updated for the same to resolve this type mismatch

asr2003 and others added 2 commits November 19, 2024 11:27
Co-authored-by: kyri-petrou <67301607+kyri-petrou@users.noreply.github.com>
@asr2003
Copy link
Contributor Author

asr2003 commented Nov 19, 2024

@kyri-petrou Done with the changes!

@asr2003 asr2003 requested a review from kyri-petrou November 20, 2024 14:18
@kyri-petrou kyri-petrou merged commit 5e3e31d into zio:series/2.x Nov 21, 2024
18 checks passed
@asr2003 asr2003 deleted the fix-ZIO#OnDone branch June 6, 2025 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider deprecating ZIO#onDone
3 participants