Provide doc comments for AsyncStream/AsyncThrowingStream #38535

invalidname · 2021-07-21T17:57:15Z

Source documentation for AsyncStream, AsyncThrowingStream, and their related types.

stdlib/public/Concurrency/AsyncStream.swift

stdlib/public/Concurrency/AsyncThrowingStream.swift

amartini51

Looks good - minor comments throughout. I read through AsyncStream.swift in detail and it looks like the API in AsyngThrowingStream.swift is largely similar. If there are symbols I missed by skimming the latter, please call them out.

stdlib/public/Concurrency/AsyncStream.swift

amartini51 · 2021-07-24T00:05:52Z

stdlib/public/Concurrency/AsyncStream.swift

+  /// Use this convenience initializer when you have an asychronous function
+  /// that can produce elements for the stream, and don't want to invoke
+  /// a continuation manually. This initializer "unfolds" your closure into
+  /// a full-blown asynchronous stream. The created stream handles adherence to


This is the first time I've seen "adherence" to a protocol. What the sentence is describing isn't quite protocol conformance, which is the usual term, but seems a bit more general.

This is kind of borrowing from the swift-evolution proposal:

This affords the case in which that unfolded function can leave the specification adherence of AsyncSequence to AsyncStream. In short the init(unfolding:onCancel:) initializer handles the terminal cases as well as the cancellation.

Conformance might be better? Since it is a behavioral requirement of AsyncSequence (not a compile time enforced property)?

Or perhaps qualify it as "well behaved conformance"?

I think we can just call it "conformance".

By analogy, supposed I made a type that implemented the methods that Collection requires, but crashed if you tried to iterate more than 3 times. That type doesn't correctly conform to the Collection protocol because a collection must support multiple iteration. Many protocols have requirements that can't be expressed in code like this.

Except in the context of discussing this sort of incorrect conformance, we shouldn't need to distinguish well-behaved conformance.

I think we can just call it "conformance".

By analogy, supposed I made a type that implemented the methods that Collection requires, but crashed if you tried to iterate more than 3 times. That type doesn't correctly conform to the Collection protocol because a collection must support multiple iteration. Many protocols have requirements that can't be expressed in code like this.

Except in the context of discussing this sort of incorrect conformance, we shouldn't need to distinguish well-behaved conformance.

@natecook1000 what do you think?

Personally, I'm fine with "conformance". I kept "adherence" as a synonym borrowed from the API proposal, because AsyncStream conforms to AsyncSequence in the first place, so it's almost a tautology to talk about conformance at all. I think the real point here is that the developer sends in a closure and this initializer provides all the other piping to serve as an AsyncStream, which in turn is an AsyncSequence. And that's what the final sentence here is about.

amartini51 · 2021-07-24T00:07:02Z

stdlib/public/Concurrency/AsyncStream.swift

+  ///     let stream = AsyncStream<Int>(
+  ///         unfolding: {
+  ///             await Task.sleep(1 * 1_000_000_000)
+  ///             return Int.random(in: 1...10)
+  ///         },
+  ///         onCancel: { @Sendable () in print ("Canceled.") }
+  ///     )


Can/should this use the multiple trailing closure syntax from Swift 5.3?

stdlib/public/Concurrency/AsyncStream.swift

Co-authored-by: Alex Martini <[email protected]>

Co-authored-by: Joseph Heck <[email protected]>

Co-authored-by: Harlan Haskins <[email protected]>

phausler · 2021-07-27T18:51:04Z

stdlib/public/Concurrency/AsyncStream.swift

+      /// yielding.
+      ///
+      /// > Note: Acting on the remaining count is valid only when calls to
+      /// yield are mutually exclusive.


The info here is more than that - the remaining is a hint to how much may be remaining, a subsequent call may race on the potentials of consuming values so the remaining is technically an underestimate (from a thread safety standpoint)

phausler · 2021-07-27T18:53:53Z

stdlib/public/Concurrency/AsyncStream.swift

      case terminated
    }

    /// A strategy that handles exhaustion of a buffer’s capacity.
    public enum BufferingPolicy {
+      /// Continue to add to the buffer, treating its capacity as infinite.


it might be good to point out as a reiteration that this is just a no limit buffer, in that it does not allocate unlimited ram ;) or something more eloquent than that...

phausler · 2021-07-27T18:54:27Z

stdlib/public/Concurrency/AsyncStream.swift

      case unbounded

      /// When the buffer is full, discard the newly received element.
-      /// This enforces keeping the specified amount of oldest values.
+      ///
+      /// This strategy enforces keeping the specified number of oldest values.


might want to add an "at most" to qualify the "specified number"

phausler · 2021-07-27T18:54:37Z

stdlib/public/Concurrency/AsyncStream.swift

      case bufferingOldest(Int)

      /// When the buffer is full, discard the oldest element in the buffer.
-      /// This enforces keeping the specified amount of newest values.
+      ///
+      /// This strategy enforces keeping the specified number of newest values.


ditto here on the "at most"

phausler · 2021-07-27T18:56:48Z

stdlib/public/Concurrency/AsyncStream.swift

+    /// needed cleanup in the cancellation handler. After reaching a terminal
+    /// state, the `AsyncStream` disposes of the callback.


the closure internally is set to nil, removing any references it has to it.

phausler · 2021-07-27T18:57:49Z

stdlib/public/Concurrency/AsyncStream.swift

+  ///
+  /// - Parameter elementType: The type of element the `AsyncStream`
+  ///   produces.
+  /// - Parameter limit: The maximum number of elements to hold in the buffer.


it is no longer called limit, it is now bufferingPolicy

phausler · 2021-07-27T18:59:45Z

stdlib/public/Concurrency/AsyncStream.swift

+  ///
+  ///     let stream = AsyncStream<Int>(Int.self,
+  ///                                   bufferingPolicy: .bufferingNewest(5)) { continuation in
+  ///             Task.detached {


Im not sure how keepRunning can be safe (at least easily...) and likely that should be set to false in the onTermination

Would you recommend a while true here, instead? That would prevent us from ever reaching continuation.finish(). Maybe it would be better pedagogy to change the stream to use a for-in loop to produce 100 random numbers, one per second, and then finish.

@phausler Changed this to a for-in loop. Does this look OK now?

phausler · 2021-07-27T19:00:24Z

stdlib/public/Concurrency/AsyncStream.swift

+  /// Use this convenience initializer when you have an asychronous function
+  /// that can produce elements for the stream, and don't want to invoke
+  /// a continuation manually. This initializer "unfolds" your closure into
+  /// a full-blown asynchronous stream. The created stream handles adherence to


Conformance might be better? Since it is a behavioral requirement of AsyncSequence (not a compile time enforced property)?

Or perhaps qualify it as "well behaved conformance"?

phausler · 2021-07-27T19:39:34Z

stdlib/public/Concurrency/AsyncThrowingStream.swift

    ///
    /// This can be called more than once and returns to the caller immediately
-    /// without blocking for any awaiting consumption from the iteration.
+    /// without blocking for any awaiting consuption from the iteration.


consumption meaning the consuming of values. this is a spelling mistake?

chuckdude

Edits and comments throughout. I think there's one FIXME in the first file (AsyncStream).

stdlib/public/Concurrency/AsyncStream.swift

chuckdude · 2021-08-17T16:24:38Z

stdlib/public/Concurrency/AsyncStream.swift

+/// property, return an `AsyncStream`, whose `build` closure -- called at
+/// runtime to create the stream -- does the following:
+///
+/// 1. Creates a `QuakeMonitor` instance.


This should be a bulleted list, not a numbered list.

I thought we preferred numbered lists for steps performed in sequence?

The preceding text isn't clear that these happen in sequence. If that's the case, then maybe tweak the lead.

chuckdude · 2021-08-17T16:26:18Z

stdlib/public/Concurrency/AsyncStream.swift

+  /// The closure you provide to the `AsyncStream` in
+  /// `init(_:bufferingPolicy:_:)` receives an instance of this type when
+  /// invoked. Use this continuation to provide elements to the stream by
+  /// calling one of the `yield` methods, then terminate the stream normally by


Suggested change

/// calling one of the `yield` methods, then terminate the stream normally by

/// calling one of the `yield` methods, then terminate the stream by

You don't need the adverb here.

I kind of think we do; terminate "normally" means to end the stream without throwing an error. If we did throw an error here, we'd say "terminate the stream with an error".

stdlib/public/Concurrency/AsyncThrowingStream.swift

stdlib/public/Concurrency/AsyncStream.swift

Co-authored-by: Chuck Toporek <[email protected]>

chuckdude

OK to go.

chuckdude · 2021-08-18T23:23:10Z

stdlib/public/Concurrency/AsyncStream.swift

+/// property, return an `AsyncStream`, whose `build` closure -- called at
+/// runtime to create the stream -- does the following:
+///
+/// 1. Creates a `QuakeMonitor` instance.


The preceding text isn't clear that these happen in sequence. If that's the case, then maybe tweak the lead.

stdlib/public/Concurrency/AsyncStream.swift

chuckdude

Approved with changes. Some typo fixes and what looks like edits from the first pass that were skipped.

stdlib/public/Concurrency/AsyncStream.swift

stdlib/public/Concurrency/AsyncThrowingStream.swift

stdlib/public/Concurrency/AsyncStream.swift

stdlib/public/Concurrency/AsyncThrowingStream.swift

stdlib/public/Concurrency/AsyncStream.swift

stdlib/public/Concurrency/AsyncThrowingStream.swift

chuckdude · 2021-08-20T20:58:04Z

stdlib/public/Concurrency/AsyncThrowingStream.swift

+  /// Use this convenience initializer when you have an asychronous function
+  /// that can produce elements for the stream, and don't want to invoke
+  /// a continuation manually. This initializer "unfolds" your closure into
+  /// a full-blown asynchronous stream. The created stream handles adherence to


Suggested change

/// a full-blown asynchronous stream. The created stream handles adherence to

/// an asynchronous stream. The created stream handles adherence to

Don't use "full-blown" as it's jargon that can cause issues for localization.

Co-authored-by: Chuck Toporek <[email protected]>

amartini51 · 2021-08-23T20:51:34Z

@swift-ci Please test.

swift-ci · 2021-08-23T21:02:53Z

Build failed
Swift Test OS X Platform
Git Sha - 717d5c6

shahmishal · 2021-08-24T16:50:37Z

@swift-ci Please test macOS

Cherry pick #38535, 83fa59d.

Chris Adamson added 6 commits July 20, 2021 13:52

First draft of doc comments for AsyncStream.

91537c0

Source docs for AsyncThrowingStream.

db293ae

Async(Throwing)Stream style fixups.

f6e3df8

Style fixes to Async(Throwing)Stream docs.

771b22d

Correct parameter name in AsyncStream doc comments

d5ff5ee

Improve code snippet in AsyncThrowingStream docs.

be6d761

invalidname assigned parkera, phausler and amartini51 Jul 21, 2021

invalidname requested a review from a team as a code owner July 21, 2021 17:57

invalidname assigned bendjones Jul 21, 2021

harlanhaskins reviewed Jul 23, 2021

View reviewed changes

stdlib/public/Concurrency/AsyncStream.swift Outdated Show resolved Hide resolved

heckj reviewed Jul 23, 2021

View reviewed changes

stdlib/public/Concurrency/AsyncStream.swift Outdated Show resolved Hide resolved

heckj reviewed Jul 23, 2021

View reviewed changes

stdlib/public/Concurrency/AsyncThrowingStream.swift Outdated Show resolved Hide resolved

amartini51 requested changes Jul 24, 2021

View reviewed changes

invalidname and others added 7 commits July 25, 2021 20:28

Apply Alex's suggestions from code review

cf123b8

Co-authored-by: Alex Martini <[email protected]>

Apply Joe H's suggestions from code review

949c357

Co-authored-by: Joseph Heck <[email protected]>

Apply Harlan's suggestions from code review

003b013

Co-authored-by: Harlan Haskins <[email protected]>

Fix duplicate words in doc comment.

4d8efaa

Address more of @amartini51's doc feedback.

83624ef

Reflow code snippet in doc comment.

8201bda

Use multiple trailing closures in doc snippet.

e6a2e17

phausler reviewed Jul 27, 2021

View reviewed changes

Chris Adamson added 4 commits July 29, 2021 11:15

Apply some doc comment feedback from @phausler.

ba14cf6

Correct note syntax for AsyncStream doc comments.

3e3538b

Update code snippet in docs for AsyncStream.init

3ed8f9f

Add AsyncStream.Continuation escaping note.

6b62146

invalidname changed the base branch from release/5.5 to release/5.5-08092021 August 11, 2021 01:57

chuckdude requested changes Aug 17, 2021

View reviewed changes

Apply Chuck's suggestions from code review

c11034c

Co-authored-by: Chuck Toporek <[email protected]>

Apply edits to Async(Throwing)Stream doc comments.

5ce6d8f

chuckdude approved these changes Aug 18, 2021

View reviewed changes

Docs: make explicit that stream closure uses steps

b1d2e24

chuckdude reviewed Aug 20, 2021

View reviewed changes

stdlib/public/Concurrency/AsyncStream.swift Outdated Show resolved Hide resolved

chuckdude approved these changes Aug 20, 2021

View reviewed changes

Incorporate spelling & wording fixes from editorial review.

717d5c6

Co-authored-by: Chuck Toporek <[email protected]>

amartini51 approved these changes Aug 23, 2021

View reviewed changes

najacque approved these changes Aug 25, 2021

View reviewed changes

najacque merged commit 83fa59d into release/5.5-08092021 Aug 25, 2021

najacque deleted the asyncstream_doc_comments_80515639 branch August 25, 2021 00:47

amartini51 added a commit that referenced this pull request Aug 25, 2021

Provide doc comments for AsyncStream/AsyncThrowingStream

d195e34

Cherry pick #38535, 83fa59d.

amartini51 mentioned this pull request Aug 25, 2021

Cherry-pick docs changes for Swift 5.5 #39038

Merged

amartini51 mentioned this pull request Nov 10, 2021

Cherry-pick concurrency docs #40115

Merged

		/// needed cleanup in the cancellation handler. After reaching a terminal
		/// state, the `AsyncStream` disposes of the callback.

	/// calling one of the `yield` methods, then terminate the stream normally by
	/// calling one of the `yield` methods, then terminate the stream by

	/// a full-blown asynchronous stream. The created stream handles adherence to
	/// an asynchronous stream. The created stream handles adherence to

Provide doc comments for AsyncStream/AsyncThrowingStream #38535

Provide doc comments for AsyncStream/AsyncThrowingStream #38535

Uh oh!

Conversation

invalidname commented Jul 21, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amartini51 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amartini51 Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

invalidname Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

invalidname Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chuckdude left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

amartini51 Jul 27, 2021 •

edited

Loading

invalidname Jul 29, 2021 •

edited

Loading

invalidname Jul 29, 2021 •

edited

Loading