-
-
Notifications
You must be signed in to change notification settings - Fork 72
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 NIOThrowingAsyncSequenceProducer #317
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #317 +/- ##
==========================================
- Coverage 43.52% 41.76% -1.76%
==========================================
Files 115 115
Lines 9763 9408 -355
==========================================
- Hits 4249 3929 -320
+ Misses 5514 5479 -35
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Glad this all worked out perfectly. I left a small comment inline
case .asyncSequence(let consumer, let dataSource): | ||
let error = PSQLError.connectionClosed | ||
case .asyncSequence(let source, let dataSource): | ||
let error = CancellationError() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully agree with the usage of CancellationError
here. It should only get thrown if the task is cancelled; however, in your code path it code be thrown when the source terminated the sequence where this would be quite confusing for the user. In general, you don't have to call source.finish
as a result of didTerminate
at all.
Motivation
We should use components from NIO where possible and not rely on our own. Learnings from PostgresNIO have influenced the new very shiny
NIOThrowingAsyncSequenceProducer
. We should adopt sooner rather than later.Changes
NIOThrowingAsyncSequenceProducer
and drop our ownAsyncStreamConsumer
Result
We support task cancellation on
iterator.next()