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

Fix edge cases with AsyncIterable protocol #729

Merged
merged 5 commits into from
Jul 31, 2023
Merged

Conversation

srikrsna-buf
Copy link
Member

@srikrsna-buf srikrsna-buf commented Jul 25, 2023

This PR addresses a couple of issues with AsyncIterable usage:

  • Call and propagate AsyncIterator's return for request iterables. This ensures request iterables are cleaned up properly when server returns early or with an error.
  • Tweaks the Transport's stream method to accept PartialMessage instead of concrete ones similar to the unary method. This helps us normalize messages in one place (run-call.ts).
  • Omit return/throw from the response iterables returned by transports/clients.
  • For bidi, start the request as soon as the client calls the method as opposed to waiting for the first read on the response. This is required for Update the WritableIterable behavior #724 to work properly.
  • Extend tests for promise client and run call functions to cover these cases.

@srikrsna-buf srikrsna-buf requested a review from timostamm July 25, 2023 23:43
srikrsna-buf

This comment was marked as resolved.

@srikrsna-buf srikrsna-buf changed the title Call and propagate AsyncIterator's return in Transports Fix edge cases with AsyncIterable protocol Jul 26, 2023
@srikrsna-buf srikrsna-buf marked this pull request as ready for review July 26, 2023 23:38
Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup. Let's do some smoke tests to verify the DX and follow up with changes as necessary, but definitely LGTM.

@@ -50,6 +50,6 @@ export interface Transport {
signal: AbortSignal | undefined,
timeoutMs: number | undefined,
header: HeadersInit | undefined,
input: AsyncIterable<I>
input: AsyncIterable<PartialMessage<I>>
Copy link
Member

Choose a reason for hiding this comment

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

A breaking change for implementors of Transport, but stream() should have accepted partial messages in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants