Allow to finish() GRPCAsyncRequestStreamWriter sync, non-throwing
#1504
Conversation
| @@ -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. | |||
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
I'm open to removing those if we're confident that nothing breaks.
There was a problem hiding this comment.
Checked that for the internal calls to finish() 
…rpc#1504) * Add sync method variant for finishing RequestStreamWriter * Adjust call sites to use sync RequestStreamWriter finish * Remove async variant of GRPCAsyncRequestStreamWriter's .finish() Co-authored-by: George Barnett <gbarnett@apple.com>
…rpc#1504) * Add sync method variant for finishing RequestStreamWriter * Adjust call sites to use sync RequestStreamWriter finish * Remove async variant of GRPCAsyncRequestStreamWriter's .finish() Co-authored-by: George Barnett <gbarnett@apple.com>
Implements #1503
The current implementation of
finish()works sync+non-throwing. Therefore the annotationsasync throwsare superfluous. To keep the API stable this PR adds an additional sync, non-throwingfinish()method.