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.

Sign up for GitHub

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

Allow to finish() GRPCAsyncRequestStreamWriter sync, non-throwing #1504

Merged
merged 4 commits into from
Oct 19, 2022
Merged

Allow to finish() GRPCAsyncRequestStreamWriter sync, non-throwing #1504

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f91f6d3
Add sync method variant for finishing RequestStreamWriter
schwmi Oct 14, 2022
116fbe8
Adjust call sites to use sync RequestStreamWriter finish
schwmi Oct 14, 2022
79107d3
Merge branch 'main' into improvement/sync-writer-finish
glbrntt Oct 19, 2022
6e52439
Remove async variant of GRPCAsyncRequestStreamWriter's .finish()
schwmi Oct 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Sources/Examples/RouteGuide/Client/RouteGuideClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ extension RouteGuideExample {
}
}

try await recordRoute.requestStream.finish()
recordRoute.requestStream.finish()
let summary = try await recordRoute.response

print(
Expand Down Expand Up @@ -188,7 +188,7 @@ extension RouteGuideExample {
try await Task.sleep(nanoseconds: UInt64.random(in: UInt64(2e8) ... UInt64(1e9)))
}

try await routeChat.requestStream.finish()
routeChat.requestStream.finish()
}

// Add a task to print each message received on the response stream.
Expand Down
5 changes: 5 additions & 0 deletions Sources/GRPC/AsyncAwaitSupport/GRPCAsyncRequestStreamWriter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,11 @@ public struct GRPCAsyncRequestStreamWriter<Request: Sendable>: Sendable {
self.asyncWriter.finish()
}

/// Sync variant for finishing the request stream for the RPC. This must be called when there are no more requests to be sent.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The CI is emitting a bunch of failures because these calls now appear to be ambiguous (which is a little surprising):

/__w/grpc-swift/grpc-swift/Sources/GRPC/AsyncAwaitSupport/GRPCClient+AsyncAwaitSupport.swift:424:11: error: expression is 'async' but is not marked with 'await'
          call.requestStream.finish()
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
          await

Could you try marking the async throwing version as @available(*, deprecated)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, this makes sense: I'm not surprised here. The compiler will prefer the async one in async contexts. I think the correct fix is @disfavoredOverload, though I don't know if it can override the preference for the async method.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose that makes sense. I was also wondering about @disfavoredOverload but was hoping deprecating the async variant may implicitly disfavour it (and there's no reason to use the async version, so deprecating it makes sense).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried out both approaches with @_disfavoredOverload and also @available(*, deprecated). Unfortunately seems both cannot overwrite the preference for async.
Think either we prefix the sync version somehow (like syncFinish) so that it can be shipped in a minor version update or we hold this back until the next major version bump.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think our original assertion that removing the async throws from finish is API breaking may be incorrect.

swift package diagnose-api-breaking-changes does not detect any breaking changes when removing async throws (it does if you add them back). The side effect of removing them is warnings for unused occurrence of try and await.

So -- unless I'm missing something (cc @Lukasa) -- we should just be able to remove async throws from the existing finish().

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm open to removing those if we're confident that nothing breaks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked that for the internal calls to finish() Bildschirmfoto 2022-10-19 um 17 18 29

public func finish() {
self.asyncWriter.finish()
}

/// Finish the request stream for the RPC with the given error.
internal func finish(_ error: Error) {
self.asyncWriter.finish(error: error)
Expand Down
4 changes: 2 additions & 2 deletions Sources/GRPC/AsyncAwaitSupport/GRPCClient+AsyncAwaitSupport.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ extension GRPCClient {
for try await request in requests {
try await call.requestStream.send(request)
}
try await call.requestStream.finish()
call.requestStream.finish()
} catch {
// If we throw then cancel the call. We will rely on the response throwing an appropriate
// error below.
Expand Down Expand Up @@ -452,7 +452,7 @@ extension GRPCClient {
for try await request in requests {
try await call.requestStream.send(request)
}
try await call.requestStream.finish()
call.requestStream.finish()
} onCancel: {
call.cancel()
}
Expand Down
4 changes: 2 additions & 2 deletions Tests/GRPCTests/AsyncAwaitSupport/AsyncClientTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ final class AsyncClientCancellationTests: GRPCTestCase {
let collect = echo.makeCollectCall()
// Send and close.
try await collect.requestStream.send(.with { $0.text = "foo" })
try await collect.requestStream.finish()
collect.requestStream.finish()

// Await the response and status.
_ = try await collect.response
Expand Down Expand Up @@ -294,7 +294,7 @@ final class AsyncClientCancellationTests: GRPCTestCase {
let update = echo.makeUpdateCall()
// Send and close.
try await update.requestStream.send(.with { $0.text = "foo" })
try await update.requestStream.finish()
update.requestStream.finish()

// Await the response and status.
let responseCount = try await update.responseStream.count()
Expand Down
6 changes: 3 additions & 3 deletions Tests/GRPCTests/AsyncAwaitSupport/AsyncIntegrationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ final class AsyncIntegrationTests: GRPCTestCase {
try await collect.requestStream.send(.with { $0.text = "boyle" })
try await collect.requestStream.send(.with { $0.text = "jeffers" })
try await collect.requestStream.send(.with { $0.text = "holt" })
try await collect.requestStream.finish()
collect.requestStream.finish()

let initialMetadata = try await collect.initialMetadata
initialMetadata.assertFirst("200", forName: ":status")
Expand Down Expand Up @@ -149,7 +149,7 @@ final class AsyncIntegrationTests: GRPCTestCase {
XCTAssertEqual(response, "Swift echo update (\(i)): \(name)")
}

try await update.requestStream.finish()
update.requestStream.finish()

// This isn't right after we make the call as servers are not guaranteed to send metadata back
// immediately. Concretely, we don't send initial metadata back until the first response
Expand Down Expand Up @@ -186,7 +186,7 @@ final class AsyncIntegrationTests: GRPCTestCase {
_ = try await update.responseStream.first(where: { _ in true })
XCTAssertNoThrow(try self.server.close().wait())
self.server = nil // So that tearDown() does not call close() again.
try await update.requestStream.finish()
update.requestStream.finish()
}
}

Expand Down
4 changes: 2 additions & 2 deletions Tests/GRPCTests/AsyncAwaitSupport/InterceptorsAsyncTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ class InterceptorsAsyncTests: GRPCTestCase {
let call = self.echo.makeCollectCall(callOptions: .init())
try await call.requestStream.send(.with { $0.text = "1 2" })
try await call.requestStream.send(.with { $0.text = "3 4" })
try await call.requestStream.finish()
call.requestStream.finish()

await assertThat(
try await call.response,
Expand Down Expand Up @@ -153,7 +153,7 @@ class InterceptorsAsyncTests: GRPCTestCase {
let call = self.echo.makeUpdateCall(callOptions: .init())
try await call.requestStream.send(.with { $0.text = "1 2" })
try await call.requestStream.send(.with { $0.text = "3 4" })
try await call.requestStream.finish()
call.requestStream.finish()

var count = 0
for try await response in call.responseStream {
Expand Down
8 changes: 4 additions & 4 deletions Tests/GRPCTests/GRPCAsyncClientCallTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ class GRPCAsyncClientCallTests: GRPCTestCase {
for word in ["boyle", "jeffers", "holt"] {
try await collect.requestStream.send(.with { $0.text = word })
}
try await collect.requestStream.finish()
collect.requestStream.finish()

await assertThat(try await collect.initialMetadata, .is(.equalTo(Self.OKInitialMetadata)))
await assertThat(try await collect.response, .doesNotThrow())
Expand Down Expand Up @@ -138,7 +138,7 @@ class GRPCAsyncClientCallTests: GRPCTestCase {
try await update.requestStream.send(request)
}
try await update.requestStream.send(requests)
try await update.requestStream.finish()
update.requestStream.finish()

let numResponses = try await update.responseStream.map { _ in 1 }.reduce(0, +)

Expand All @@ -163,7 +163,7 @@ class GRPCAsyncClientCallTests: GRPCTestCase {
await assertThat(try await responseStreamIterator.next(), .is(.notNil()))
}

try await update.requestStream.finish()
update.requestStream.finish()

await assertThat(try await responseStreamIterator.next(), .is(.nil()))

Expand Down Expand Up @@ -191,7 +191,7 @@ class GRPCAsyncClientCallTests: GRPCTestCase {
try await update.requestStream.send(.with { $0.text = word })
await counter.incrementRequests()
}
try await update.requestStream.finish()
update.requestStream.finish()
}
// Get responses in a separate task.
taskGroup.addTask {
Expand Down