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

Replace AsyncBackpressuredStream with updated implementation #29

Merged

Conversation

simonjbeaumont
Copy link
Collaborator

Motivation

As a follow up to #27, we noted it would be good to align on the latest draft implementation of SE-0406 (AsyncStream with backpressure) to both pickup the latest improvements in performance and correctness, and to minimise the churn if/when this lands in the standard library or standalone package.

Modifications

In order to simplify reviewing the following modifications have been made in independent commits:

  • Add updated SE-0406 implementation as BufferedStream, incl. tests: Skim over this—it's vendored in wholesale.
  • Port the custom watermark support to BufferedStream: Skim over this—it's a 1:1 port of the logic that was added to AsyncBackpressuredStream.
  • Switch from AsyncBackpressuredStream to BufferedStream in delegate: Review this—it's a minimal change.
  • Remove AsyncBackpressuredStream and its vendored locks: Skim over this—it's removing the old implementation.

Result

No functional change, but the internal async sequence we're using should be more robust, performant, and more likely to match a future standard library type.

Test Plan

  • The new vendored BufferedStream actually comes with a much greater number of vendored tests than the previous revision.
  • Also ported the tests from this repo for the custom watermark logic.
  • All our URLSessionTransport-specific tests continue to pass.

@simonjbeaumont simonjbeaumont changed the title Sb/update async stream implementation Replace AsyncBackpressuredStream with updated implementation Nov 23, 2023
Copy link
Collaborator

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

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

lgtm. Please once this is released, double check that NIO isn't brought in during package resolution when depending on the transport as a library.

@simonjbeaumont
Copy link
Collaborator Author

lgtm. Please once this is released, double check that NIO isn't brought in during package resolution when depending on the transport as a library.

So here's an example package that depends on the source branch for this PR, showing that it doesn't clone NIO. The only new dependency this release will bring in is swift-collections.

tree
.
├── Package.swift
└── Sources
    └── OpenAPIURLSessionAdopter
        └── Example.swiftcat Package.swift
// swift-tools-version: 5.9
import PackageDescription

let package = Package(
    name: "swift-openapi-urlsession-adopter",
    platforms: [.macOS(.v12)],
    dependencies: [
        .package(
            url: "https://github.com/simonjbeaumont/swift-openapi-urlsession",
            branch: "sb/update-async-stream-implementation"
        ),
    ],
    targets: [
        .target(
            name: "OpenAPIURLSessionAdopter",
            dependencies: [
                .product(name: "OpenAPIURLSession", package: "swift-openapi-urlsession"),
            ]
        ),
    ]
)cat Sources/OpenAPIURLSessionAdopter/Example.swift
import OpenAPIURLSession

func f() {
    _ = OpenAPIURLSession.URLSessionTransport()
}swift build
Fetching https://github.com/simonjbeaumont/swift-openapi-urlsession from cache
Fetched https://github.com/simonjbeaumont/swift-openapi-urlsession (0.53s)
Fetching https://github.com/apple/swift-collections from cache
Fetching https://github.com/apple/swift-openapi-runtime from cache
Fetching https://github.com/apple/swift-http-types from cache
Fetched https://github.com/apple/swift-http-types (0.52s)
Fetched https://github.com/apple/swift-collections (0.54s)
Fetched https://github.com/apple/swift-openapi-runtime (0.65s)
Computing version for https://github.com/apple/swift-collections
Computed https://github.com/apple/swift-collections at 1.0.5 (0.50s)
Computing version for https://github.com/apple/swift-openapi-runtime
Computed https://github.com/apple/swift-openapi-runtime at 0.3.6 (0.43s)
Computing version for https://github.com/apple/swift-http-types
Computed https://github.com/apple/swift-http-types at 1.0.0 (0.52s)
Creating working copy for https://github.com/simonjbeaumont/swift-openapi-urlsession
Working copy of https://github.com/simonjbeaumont/swift-openapi-urlsession resolved at sb/update-async-stream-implementation
Creating working copy for https://github.com/apple/swift-openapi-runtime
Working copy of https://github.com/apple/swift-openapi-runtime resolved at 0.3.6
Creating working copy for https://github.com/apple/swift-collections
Working copy of https://github.com/apple/swift-collections resolved at 1.0.5
Creating working copy for https://github.com/apple/swift-http-types
Working copy of https://github.com/apple/swift-http-types resolved at 1.0.0
Building for debugging...
[84/84] Compiling OpenAPIURLSessionAdopter Example.swift
Build complete! (17.54s)cat Package.resolved | jq .pins[].location
"https://github.com/apple/swift-collections"
"https://github.com/apple/swift-http-types"
"https://github.com/apple/swift-openapi-runtime"
"https://github.com/simonjbeaumont/swift-openapi-urlsession"swift package show-dependencies
.
└── swift-openapi-urlsession<https://github.com/simonjbeaumont/swift-openapi-urlsession@unspecified>
    ├── swift-openapi-runtime<https://github.com/apple/swift-openapi-runtime@0.3.6>
    │   └── swift-http-types<https://github.com/apple/swift-http-types@1.0.0>
    └── swift-collections<https://github.com/apple/swift-collections@1.0.5>

@simonjbeaumont simonjbeaumont marked this pull request as ready for review November 23, 2023 20:21
@simonjbeaumont simonjbeaumont enabled auto-merge (squash) November 23, 2023 20:22
@simonjbeaumont simonjbeaumont added the semver/patch No public API change. label Nov 23, 2023
@simonjbeaumont
Copy link
Collaborator Author

@swift-server-bot test this please

@simonjbeaumont simonjbeaumont merged commit 9229842 into apple:main Nov 23, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants