Skip to content

Conversation

FranzBusch
Copy link
Member

Motivation

We started to add bridges from NIO to Swift Concurrency and we should provide some narrative documentation for our users how to adopt them.

Modification

Adds a new article that explains the NIO concurrency bridges.

@FranzBusch FranzBusch force-pushed the fb-concurrency-narritve-doc branch from 8934b25 to 9e1069c Compare May 15, 2023 17:02
# Motivation
We started to add bridges from NIO to Swift Concurrency and we should provide some narrative documentation for our users how to adopt them.

# Modification
Adds a new article that explains the NIO concurrency bridges.
@FranzBusch FranzBusch force-pushed the fb-concurrency-narritve-doc branch from 9e1069c to f3e0aaf Compare May 15, 2023 17:10
to close the channel once both the reading and the writing have finished.


This is how you can wrap an existing channel into a `NIOAsyncChannel`, consume the inbound data and
Copy link
Member Author

Choose a reason for hiding this comment

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

We cannot use the symbol link yet because we still use spi for the symbols

The first bridges, that NIO introduced were adding methods on ``EventLoopFuture`` and ``EventLoopPromise``
to bridge to and from Concurrency. Namely these bridges are ``EventLoopFuture/get()`` and ``EventLoopPromise/completeWithTask(_:)``.

> Warning: The future ``EventLoopFuture/get()`` method does not support task cancellation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
> Warning: The future ``EventLoopFuture/get()`` method does not support task cancellation.
> Warning: ``EventLoopFuture/get()`` does not support task cancellation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did add the words around because docc renders this as just get() which reads very weird.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see. Okay, then it's fine - with the word future though, did you mean ELF?

@FranzBusch FranzBusch requested review from Lukasa, glbrntt and gjcairo May 18, 2023 09:17
@FranzBusch
Copy link
Member Author

@gjcairo @Lukasa @glbrntt Thanks for the great feedback. I fixed all the nits and rearranged some sections to have a better story line. I also added a general guidance section in the end. Right now that only contains guidance around business logic vs protocol logic but we can extend it in the future with guidance around perf as well

Copy link
Contributor

@gjcairo gjcairo left a comment

Choose a reason for hiding this comment

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

Looking good - left one more comment.


#### Where should your code live?

Before the introduction of Swift Concurrency both, implementations of network protocols and business logic,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Before the introduction of Swift Concurrency both, implementations of network protocols and business logic,
Before the introduction of Swift Concurrency, both implementations of network protocols and business logic

try await withThrowingDiscardingTaskGroup { group in
for try await negotiationResult in serverChannel.inboundStream {
group.addTask {
do {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a catch missing or why do we use do here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Pun intended

@FranzBusch FranzBusch marked this pull request as ready for review May 18, 2023 12:52
@Lukasa Lukasa added the semver/none No version bump required. label May 22, 2023
protocolNegotiationHandlerType: NIOTypedApplicationProtocolNegotiationHandler<NegotiationResult>.self
)

try await withThrowingDiscardingTaskGroup { group in
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth mentioning withThrowingDiscardingTaskGroup is not available in macOS version of Swift 5.8, but is available in Linux?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that is the right place to mention this. This should be covered by the documentation of withThrowingDiscardingTaskGroup.

@FranzBusch
Copy link
Member Author

Merging over the nightly failure which is an allocation regression

@FranzBusch FranzBusch merged commit 2762085 into apple:main May 22, 2023
@FranzBusch FranzBusch deleted the fb-concurrency-narritve-doc branch May 22, 2023 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants