Strict Concurrency by rnro · Pull Request #98 · apple/swift-nio-examples

rnro · 2025-04-07T15:04:04Z

Motivation

Make the examples strict concurrency safe and lock-in those changes with CI.

Modifications

Resolve strict concurrency issues
Add opt-in strict concurrency settings to package manifests
Add always-on strict concurrency settings to the Xcode project for NIOSMTP when built in debug mode (!)
Remove nio-launchd from Linux CI since it requires a Darwin platform

Result

Concurrency-safe code

glbrntt · 2025-04-08T10:04:42Z

+
+        do {
+            try myChannel.pipeline.syncOperations.addHandler(myGlue, position: .after(contextForInitialData.handler))
+            _ = try partnerChannel.pipeline.syncOperations.handler(type: CloseOnErrorHandler.self)


Why are we getting the handler and throwing it away?

The previous code seemed to do effectively that

{ partnerChannel.pipeline.handler(type: CloseOnErrorHandler.self) }.flatMap { errorHandler in partnerChannel.pipeline.addHandler(partnerGlue) }

Maybe it was doing for some side-effect or maybe it was leftover code? I guess either way now it's synchronous I guess I can just delete line 98 now.

I suspect the glue handler was meant to be added before the error handler. Normally the close-on-error handler would be the last in the pipeline.

It looks like it is currently inserted after ChannelHandlerContext.handler where the context passed along after the error handler has been inserted, I can't work out from a glance at the code what that actually represents.

In my current changes I have just removed that line on 98. Should I change

try myChannel.pipeline.syncOperations.addHandler(myGlue, position: .after(contextForInitialData.handler))

to

let closeErrorHandler = try partnerChannel.pipeline.syncOperations.handler(type: CloseOnErrorHandler.self) try myChannel.pipeline.syncOperations.addHandler(myGlue, position: .before(closeErrorHandler))

glbrntt · 2025-04-08T10:07:10Z

+            peerChannel.close(mode: .all, promise: nil)
+            context.close(promise: nil)
        }
+        context.pipeline.syncOperations.removeHandler(self, promise: nil)


This was only done on the success path before

glbrntt · 2025-04-08T10:08:37Z

-            context.pipeline.removeHandler(context: $0, promise: nil)
-        }
+
+        let byteToMessageHandlerContext = try! context.pipeline.syncOperations.context(handlerType: ByteToMessageHandler<HTTPRequestDecoder>.self)


The previous code didn't blow up if the context couldn't be found

I've made this no longer blow up but only remove the handler if it can be found.

glbrntt · 2025-04-08T10:08:44Z

-        context.pipeline.context(handlerType: HTTPResponseEncoder.self).whenSuccess {
-            context.pipeline.removeHandler(context: $0, promise: nil)
-        }
+        let httpResponseEncoderContext = try! context.pipeline.syncOperations.context(handlerType: HTTPResponseEncoder.self)


I've made this no longer blow up but only remove the handler if it can be found.

glbrntt · 2025-04-08T10:13:05Z

-                return context.channel.writeAndFlush(self.wrapOutboundOut(HTTPServerResponsePart.end(nil)))
-            }.whenComplete { _ in
+                context.channel.write(HTTPServerResponsePart.body(.byteBuffer(buffer)), promise: nil)
+                _ = context.channel.writeAndFlush(HTTPServerResponsePart.end(nil))


The close was chain off of the write-and-flush before, now it isn't. It should either be chained off of it or we should pass promise: nil to writeAndFlush

glbrntt · 2025-04-08T10:14:33Z

 import Foundation

-public enum ResultType<Value, Error> {
+public enum ResultType<Value, Error>: Sendable where Value: Sendable, Error: Sendable {


This shows its age 😄

glbrntt · 2025-04-08T10:15:25Z

+        var serverConfig: ServerConfiguration? = nil
+        guard let serverConfig = serverConfig else {
+            fatalError("You need to configure an SMTP server in code.")
+        }


... this isn't gonna work.

The intention was for it to fatal error in this case. I've changed this to be a precondition failure instead which is hopefully more obvious

glbrntt · 2025-04-08T10:18:21Z

        run: echo "build-test-matrix=$(curl -s https://raw.githubusercontent.com/apple/swift-nio/main/scripts/generate_matrix.sh | bash)" >> "$GITHUB_OUTPUT"
        env:
-          MATRIX_LINUX_COMMAND: STRICT_CONCURRENCY=true SWIFT_PACKAGE_DIRECTORIES='TLSify UniversalBootstrapDemo http-responsiveness-server connect-proxy http2-client http2-server json-rpc nio-launchd' dev/build_all.sh && SWIFT_PACKAGE_DIRECTORIES='backpressure-file-io-channel' dev/build_all.sh
+          MATRIX_LINUX_COMMAND: STRICT_CONCURRENCY=true SWIFT_PACKAGE_DIRECTORIES='TLSify UniversalBootstrapDemo http-responsiveness-server connect-proxy http2-client http2-server json-rpc' dev/build_all.sh && SWIFT_PACKAGE_DIRECTORIES='backpressure-file-io-channel' dev/build_all.sh


Rather than removing it, it probably makes more sense to update the example so that when compiling on linux it's a no-op or prints a message saying it's a Darwin only or similar.

glbrntt · 2025-04-10T09:54:12Z

        context.close(promise: nil)
    }
 }
+#endif


Do you need an else which prints an warning that this is Darwin only?

I do that in main.swift - is that sufficient? 4b0befb#diff-913ea37bd603731036678b1acc7dbe1d1e08da07adf3b1521c5514c9019ccf01R30

Oh yeah, that's fine.

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

rnro added 11 commits April 7, 2025 15:35

TLSify strict concurrency

2f68fb4

UniversalBootstrapDemo strict concurrency

387f3d7

backpressure-file-io-channel strict concurrency

1b6016f

connect-proxy strict concurrency

a42bc55

http2-client strict concurrency

ea7fcfa

http2-server strict concurrency

41620b5

nio-launchd strict concurrency

c1b12eb

json-rpc strict concurrency

907e9d3

NIOSMTP strict concurrency

75cbaad

http-responsiveness-server strict concurrency

e58e8df

add -Xfrontend

3a7f831

rnro changed the title ~~Strict concurrency only~~ Strict Concurrency Apr 7, 2025

ChannelOptions.socket

fd717f7

rnro force-pushed the strict_concurrency_only branch from 22a75e9 to fd717f7 Compare April 7, 2025 15:23

rnro added 2 commits April 7, 2025 16:23

http-responsiveness-server supports tools-version 5.9

ec082cd

remove nio-launchd - darwin only

c08f4d9

rnro marked this pull request as ready for review April 7, 2025 16:38

rnro requested a review from glbrntt April 7, 2025 16:38

rnro enabled auto-merge (squash) April 7, 2025 16:39

glbrntt reviewed Apr 8, 2025

View reviewed changes

review comments

4b0befb

rnro requested a review from glbrntt April 8, 2025 12:27

glbrntt reviewed Apr 10, 2025

View reviewed changes

Update backpressure-file-io-channel/Sources/BackpressureChannelToFile…

f2532bc

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

rnro requested a review from glbrntt April 10, 2025 10:08

glbrntt approved these changes Apr 10, 2025

View reviewed changes

rnro merged commit c06a18c into main Apr 10, 2025
15 checks passed

rnro deleted the strict_concurrency_only branch April 10, 2025 11:57

Conversation

rnro commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Result

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rnro commented Apr 7, 2025 •

edited

Loading