Add AsyncBytes, AsyncLineSequence, AsyncCharacterSequence, and AsyncUnicodeScalarSequence #3036

Catfish-Man · 2021-08-11T17:04:56Z

Only the local-file versions of AsyncBytes for now

…nicodeScalarSequence

Sources/Foundation/FileHandle+Async.swift

millenomi · 2021-08-11T17:22:49Z

Sources/Foundation/URL+AsyncBytes.swift

+                        let fh = try await IOActor.default.createFileHandle(reading: url).bytes.makeAsyncIterator()
+                        buf = fh.buffer
+                    } else {
+                        // TODO: add networking support


For future reference: use _NSNonfileURLContentLoader in SCF to load using URLSession if available. Right now it only returns an in-memory NSData after all is done, but it may be sufficient here.

millenomi · 2021-08-11T17:24:41Z

@swift-ci please test

Catfish-Man · 2021-08-11T17:29:28Z

Sources/Foundation/FileHandle+Async.swift

+        public struct Iterator: AsyncIteratorProtocol {
+
+            @inline(__always) static var bufferSize: Int {
+                16384


It occurs to me this should probably be 16384 minus the size of the object header

Or maybe not so the loop can be aligned. Ugh, I'll need to dig into this at some point.

Sources/Foundation/FileHandle+Async.swift

compnerd · 2021-08-13T23:04:40Z

FYI: this also breaks on Windows: https://dev.azure.com/compnerd/swift-build/_build/results?buildId=53483&view=logs&j=ec47c80d-c7cf-5f2b-7138-13cd268bea3d&t=7a188035-f39c-599a-5e00-254b4ae08c42

Will follow up on _read after Co-authored-by: Aura Lily Vulcano <[email protected]>

millenomi · 2021-08-20T19:22:30Z

@swift-ci please test

millenomi · 2021-08-20T19:22:37Z

I missed the change.

millenomi · 2021-08-23T20:39:35Z

@swift-ci please test

millenomi · 2021-08-23T20:40:03Z

Ah, never mind, this has merge conflicts.

millenomi · 2021-08-23T20:41:31Z

@swift-ci please test

millenomi · 2021-09-01T16:18:49Z

@swift-ci please test

millenomi · 2021-09-01T18:06:52Z

@swift-ci please test

millenomi · 2021-09-01T20:12:39Z

@swift-ci please test

millenomi · 2021-09-01T21:22:42Z

Waiting for Linux build to pass before correcting the availability annotation for macOS. (Or fail.)

millenomi · 2021-09-01T22:17:19Z

Tests/Foundation/Tests/TestFileHandle+Async.swift

+            ("testSeparatorPrefixAndContinuationWithoutFin", test_seperator_prefix_and_continuation_without_fin)
+        ]
+
+        return tests


@Catfish-Man Is this what you meant here? This method was missing its termination.

millenomi · 2021-09-01T22:17:45Z

@swift-ci please test

millenomi · 2021-09-01T22:19:04Z

@swift-ci please test

millenomi · 2021-09-02T18:44:21Z

@swift-ci please test

millenomi · 2021-09-02T19:45:15Z

A little worried about this patch as we haven't even gotten to making tests run yet. @Catfish-Man do you have some time to shepherd it actively?

millenomi · 2021-09-02T19:45:35Z

(Hopefully this one is the run that gets to unit tests.)

millenomi · 2021-09-03T17:05:19Z

The tests aren't running because they weren't listed in the main.

millenomi · 2021-09-03T17:17:27Z

@swift-ci please test

millenomi · 2021-10-01T04:48:27Z

@swift-ci please test

gwynne · 2021-10-13T07:35:06Z

Sources/Foundation/FileHandle+Async.swift

+#if canImport(Darwin)
+            let read = Darwin.read
+#elseif canImport(Glibc)
+            let read = Glibc.read
+#elseif canImport(CRT)
+            let read = CRT._read
+#endif


Is there any utility in s-c-f for avoiding doing this everywhere read(2) is used? If not, is there a specific reason otherwise? (Offhand, "want to just use System when possible someday" comes to mind.)

Citation: https://github.com/apple/swift-nio/blob/main/Sources/NIOPosix/System.swift#L60-L80 et al

System is a package, which means we would have to lower it into the distribution to use it ourselves :(

millenomi · 2021-10-27T01:32:59Z

@swift-ci please test

millenomi · 2021-11-04T18:50:11Z

@swift-ci please test

MaxDesiatov · 2022-01-25T13:42:54Z

@swift-ci please test

Gerzer · 2022-02-16T04:06:57Z

I understand that this pull request is holding up the implementation of the new concurrency APIs in URL and friends. Is there any update on when it might be merged and when work might begin on those new APIs? Is there any need for additional help with these implementations?

YOCKOW · 2022-04-13T07:40:51Z

Tests/Foundation/main.swift

@@ -124,6 +124,7 @@ var allTestCases = [
    testCase(TestUnitVolume.allTests),
    testCase(TestNSLock.allTests),
    testCase(TestNSSortDescriptor.allTests),
+    testCase(TestFileHandleAsync.allTests),


(Let me leave a comment before the test logs would be gone.)
Build failure here:
error: invalid conversion from 'async' function of type '() async throws -> ()' to synchronous function type '() throws -> Void'

waterskier2007 · 2022-04-25T19:32:40Z

@YOCKOW so it looks like there just needs to be a utility function written to wrap an async test case since the current implementation only takes non-async functions. Is that correct?

waterskier2007 · 2022-04-25T19:33:26Z

Current implementation here https://github.com/apple/swift-corelibs-foundation/blob/a-sink-eye-oh/Tests/Foundation/Utilities.swift#L14

YOCKOW · 2022-04-27T08:42:49Z

Current implementation here https://github.com/apple/swift-corelibs-foundation/blob/a-sink-eye-oh/Tests/Foundation/Utilities.swift#L14

I think, unfortunately, no. It is used only when DARWIN_COMPATIBILITY_TESTS is defined.
A function testCase is implemented in XCTest, which is open source on Linux:
https://github.com/apple/swift-corelibs-xctest/blob/56501e765303ccac2f104a6e45cfcd6e733cc74e/Sources/XCTest/Public/XCTestCase.swift#L302

waterskier2007 · 2022-04-27T10:50:19Z

Alright, so then we need to PR an async version of testCase in XCTest and utilize that in this current PR?

YOCKOW · 2022-04-29T02:13:10Z

Alright, so then we need to PR an async version of testCase in XCTest and utilize that in this current PR?

Or, we can use asyncTest.

MPLew-is · 2022-07-07T00:23:23Z

Is there anything that a novice community member like me can do to help land this? We're building Lambdas on AWS in Swift and would love to be able to get full AsyncBytes support.

iCharlesHu · 2022-08-09T17:49:40Z

@swift-ci please test

parkera · 2022-08-09T20:15:14Z

Is there anything that a novice community member like me can do to help land this? We're building Lambdas on AWS in Swift and would love to be able to get full AsyncBytes support.

The Swift Async Algorithms package has an updated version of this as AsyncBufferedByteIterator.

MPLew-is · 2022-08-11T20:17:00Z

@parkera thanks for the link, that should help at least bridge the gap a little - however, it would still be incredibly nice to be able to land this Foundation version, especially for its integration with FileHandle. Right now, doing cross-platform async file I/O is painful since Linux is behind the Mac version.

Zollerboy1 · 2022-08-19T15:13:46Z

Sources/Foundation/FileHandle+Async.swift

+        // this is not incredibly effecient but it is the best we have
+        guard let data = try handle.read(upToCount: buffer.count) else {
+            return 0
+        }
+        data.copyBytes(to: buffer)
+        return data.count


Is there a specific reason, handle.readabilityHandler isn't used here in conjunction with withUnsafeThrowingContinuation? Since this is still not part of Foundation on Linux, I wanted to include this code in my package for the moment and just wondered if it was better to use readabilityHandler because this function is blocking otherwise.

readabilityHandler is less useful than you'd hope for regular files. They're just always readable.

But a FileHandle can also be the output of Process, and it may need to wait for new bytes. So the thread will be locked with read(upToCount:). I think readabilityHandler will be a better option that we can wrap into continuation

macserv · 2022-12-02T04:06:40Z

Sources/Foundation/AsyncLineSequence.swift

+                    continue
+                case _LF..<_CR:
+                    guard let result = yield() else {
+                        continue


Forgive me if I'm oversimplifying... there's a lot going on here. 😊 It looks like this completely removes empty lines, which seems suboptimal, given that...

blank lines are meaningful in countless data applications, and

it's easy enough to add your own isEmpty filter on the sequence if you really do want to strip them out.

Based on this, the default behavior should probably be to include empty lines (i.e., change continue to return "" in the _CR and _LF cases).

If in doubt, please cross-check with Foundation's behavior. It needs to have the same semantics, else we widen the (already large) gap between APPLE and !APPLE.

Ah, yeah, good point. This does match Foundation's behavior, so the issue would be against Apple, not this code. Thanks for the check!

Add AsyncBytes, AsyncLineSequence, AsyncCharacterSequence, and AsyncU…

4860800

…nicodeScalarSequence

Catfish-Man requested review from millenomi and compnerd August 11, 2021 17:04

Catfish-Man self-assigned this Aug 11, 2021

millenomi reviewed Aug 11, 2021

View reviewed changes

Sources/Foundation/FileHandle+Async.swift Show resolved Hide resolved

millenomi reviewed Aug 11, 2021

View reviewed changes

Catfish-Man commented Aug 11, 2021

View reviewed changes

millenomi reviewed Aug 11, 2021

View reviewed changes

Sources/Foundation/FileHandle+Async.swift Show resolved Hide resolved

millenomi reviewed Aug 11, 2021

View reviewed changes

Sources/Foundation/FileHandle+Async.swift Outdated Show resolved Hide resolved

Apply suggestions from code review

4fb48c4

Will follow up on _read after Co-authored-by: Aura Lily Vulcano <[email protected]>

Merge branch 'main' into a-sink-eye-oh

557cf62

throws async -> async throws

f73d3dd

Add missing brace.

bd0c1e6

Fix missing return and class name collision.

f0c813e

millenomi reviewed Sep 1, 2021

View reviewed changes

Add missing availability.

2b57290

Remove calls; these are passed as closures.

a061294

millenomi added 2 commits September 3, 2021 10:06

Include new unit tests in the test list.

c812452

Experiment in avoiding the macOS error by using explicit availability.

f15da19

millenomi mentioned this pull request Sep 8, 2021

[REVIEW] Add Async Support to XCTest swiftlang/swift-corelibs-xctest#326

Merged

gwynne reviewed Oct 13, 2021

View reviewed changes

YOCKOW reviewed Apr 13, 2022

View reviewed changes

swift-ci mentioned this pull request May 5, 2022

[SR-15187] URLSession missing async APIs in Swift 5.5 #3205

Closed

Zollerboy1 reviewed Aug 19, 2022

View reviewed changes

macserv reviewed Dec 2, 2022

View reviewed changes

zunda-pixel mentioned this pull request Jul 15, 2024

support URLSession concurrency method in Swift 6(Linux, Windows) apple/swift-http-types#58

Merged

Zollerboy1 mentioned this pull request Dec 3, 2024

swift6 warnings about passing raw mutable buffers? Zollerboy1/SwiftCommand#10

Open

Add AsyncBytes, AsyncLineSequence, AsyncCharacterSequence, and AsyncUnicodeScalarSequence #3036

Are you sure you want to change the base?

Add AsyncBytes, AsyncLineSequence, AsyncCharacterSequence, and AsyncUnicodeScalarSequence #3036

Conversation

Catfish-Man commented Aug 11, 2021

Choose a reason for hiding this comment

millenomi commented Aug 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compnerd commented Aug 13, 2021

millenomi commented Aug 20, 2021

millenomi commented Aug 20, 2021

millenomi commented Aug 23, 2021

millenomi commented Aug 23, 2021

millenomi commented Aug 23, 2021

millenomi commented Sep 1, 2021

millenomi commented Sep 1, 2021

millenomi commented Sep 1, 2021

millenomi commented Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

millenomi commented Sep 1, 2021

millenomi commented Sep 1, 2021

millenomi commented Sep 2, 2021

millenomi commented Sep 2, 2021

millenomi commented Sep 2, 2021

millenomi commented Sep 3, 2021

millenomi commented Sep 3, 2021

millenomi commented Oct 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millenomi commented Oct 27, 2021

millenomi commented Nov 4, 2021

MaxDesiatov commented Jan 25, 2022

Gerzer commented Feb 16, 2022 • edited Loading

YOCKOW Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

waterskier2007 commented Apr 25, 2022 • edited Loading

waterskier2007 commented Apr 25, 2022

YOCKOW commented Apr 27, 2022

waterskier2007 commented Apr 27, 2022 • edited Loading

YOCKOW commented Apr 29, 2022

MPLew-is commented Jul 7, 2022

iCharlesHu commented Aug 9, 2022

parkera commented Aug 9, 2022

MPLew-is commented Aug 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millenomi commented Sep 1, 2021 •

edited

Loading

Gerzer commented Feb 16, 2022 •

edited

Loading

YOCKOW Apr 13, 2022 •

edited

Loading

waterskier2007 commented Apr 25, 2022 •

edited

Loading

waterskier2007 commented Apr 27, 2022 •

edited

Loading