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

fix(async): abortable should not change original outputs #5609

Merged
merged 7 commits into from
Aug 5, 2024

Conversation

raaymax
Copy link
Contributor

@raaymax raaymax commented Aug 1, 2024

Related to this PR: #5560
And this issue #3518

@raaymax raaymax requested a review from kt3k as a code owner August 1, 2024 20:18
@github-actions github-actions bot added the async label Aug 1, 2024
Copy link

codecov bot commented Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.01%. Comparing base (9d87893) to head (81996c8).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5609      +/-   ##
==========================================
- Coverage   96.38%   96.01%   -0.37%     
==========================================
  Files         466      466              
  Lines       37588    37590       +2     
  Branches     5541     5510      -31     
==========================================
- Hits        36228    36091     -137     
- Misses       1318     1457     +139     
  Partials       42       42              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +204 to +205
await abortableIterator.return(321),
await normalIterator.return(321),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is still small difference in types of return function argument between normalIterator and abortableIterator.
But this can't be fixed without changing AsyncIterable<T> type.
denoland/deno#24837

Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

Nice work. Can you please also move the return() from the catch block to a finally block? I believe that should be the correct way to do it.

@raaymax
Copy link
Contributor Author

raaymax commented Aug 2, 2024

As you wish but this way, the function will be called twice if no exception occurs.

@iuioiua
Copy link
Contributor

iuioiua commented Aug 2, 2024

In that case, we'd likely need to at a let returned = false before the try/catch, set it do true when done is truthy, then check if that value is true in the finally block. We should also have a test case for that too.

@kt3k
Copy link
Member

kt3k commented Aug 2, 2024

In that case, we'd likely need to at a let returned = false before the try/catch, set it do true when done is truthy, then check if that value is true in the finally block.

Sounds like the same thing can be achieved by calling it?.return() in catch block?

@raaymax
Copy link
Contributor Author

raaymax commented Aug 2, 2024

I thought so too and thats exactly what I did in the first place.
In my opinion, using a boolean flag for this purpose is just ugly, but it's not my decision to make.
You need to decide on one solution here.

@raaymax raaymax requested a review from iuioiua August 2, 2024 08:45
Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

Apologies. What the catch previously achieved didn't click with me. I've reverted that change now. Now, LGTM. Thank you.

@iuioiua iuioiua enabled auto-merge (squash) August 5, 2024 05:30
@iuioiua iuioiua merged commit ed19729 into denoland:main Aug 5, 2024
11 checks passed
@raaymax raaymax deleted the fix-abortable-return branch August 5, 2024 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants