-
Notifications
You must be signed in to change notification settings - Fork 35
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
Replace AsyncBackpressuredStream with updated implementation #29
Conversation
Signed-off-by: Si Beaumont <beaumont@apple.com>
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. 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.swift
❯ cat Package.swift
// swift-tools-version: 5.9
import PackageDescription
let package = Package(
name: "swift-openapi-urlsession-adopter",
platforms: [.macOS(.v12)],
dependencies: [
.package(
url: "http://222.178.203.72:19005/whst/63/=fhsgtazbnl//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> |
@swift-server-bot test this please |
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 toAsyncBackpressuredStream
.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
BufferedStream
actually comes with a much greater number of vendored tests than the previous revision.