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

Throw CancellationError instead of returning nil during early cancellation. #2401

Merged
merged 14 commits into from Apr 11, 2023

Conversation

dnadoba
Copy link
Member

@dnadoba dnadoba commented Apr 11, 2023

Motivation:

Follow up PR for #2399

We currently still return nil if the current Task is canceled before the first call to NIOThrowingAsyncSequenceProducer.AsyncIterator.next() but it should throw CancellationError too.

In addition, the generic Failure type turns out to be a problem. Just throwing a CancellationError without checking that Failure type is any Swift.Error or CancellationError introduced a type safety violation as we throw an unrelated type.

Modifications:

  • throw CancellationError on eager cancellation
  • deprecates the generic Failure type of NIOThrowingAsyncSequenceProducer. It now must always be any Swift.Error. For backward compatibility we will still return nil if Failure is not any Swift.Error or CancellationError.

Result:

CancellationError is now correctly thrown instead of returning nil on eager cancelation. Generic Failure type is deprecated.

…terator.next()` is cancelled instead of returning `nil`
…ation.

This also deprecates the generic `Failure` type of `NIOThrowingAsyncSequenceProducer` and it now must always be `any Swift.Error`. Otherwise we couldn’t throw `CancellationError` on cancellation and again need to return nil. For backward compatibility we will still return nil if `Failure` is not `any Swift.Error` or `CancellationError`.
# Conflicts:
#	Sources/NIOCore/AsyncSequences/NIOThrowingAsyncSequenceProducer.swift
@dnadoba dnadoba added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Apr 11, 2023
dnadoba and others added 4 commits April 11, 2023 12:15
…r.swift

Co-authored-by: George Barnett <gbarnett@apple.com>
…r.swift

Co-authored-by: George Barnett <gbarnett@apple.com>
…r.swift

Co-authored-by: George Barnett <gbarnett@apple.com>
…r.swift

Co-authored-by: George Barnett <gbarnett@apple.com>
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

We clearly have no tests for the typed error case, as we haven't had to deprecate anything. Can you add tests for that case that confirm we appropriately do not throw CancellationError in them, thereby testing the new function?

@dnadoba
Copy link
Member Author

dnadoba commented Apr 11, 2023

@Lukasa tests added in 28e4c70

@dnadoba dnadoba requested a review from Lukasa April 11, 2023 15:25
backPressureStrategy: backPressureStrategy,
delegate: delegate
)
let sequence = new.sequence
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make sure to use withExtendedLifetime to preserve the producer side? That'll help me be confident that this code doesn't have this effect because the producer is being dropped.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in c7863f6
We can't wrap the whole thing in withExtendedLifetime because we call an async method and the closure withExtendedLifetime takes isn't async. (another use case for reasync)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I'm happy with the patch as written. More broadly, if you ever want to say "I need a value to live to this point in the code", the idiom has ended up becoming to just write withExtendedLifetime(theValue) { }. Your version works too, so I'm happy with this as-is, but just an FYI for the future that you could have simply written that at the bottom of the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, nice. Thank you!

@dnadoba dnadoba enabled auto-merge (squash) April 11, 2023 15:36
@dnadoba dnadoba merged commit e0cc6dd into apple:main Apr 11, 2023
5 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants