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 UniversalHandlers resolving the response before headers are complete #588

Merged
merged 5 commits into from
Apr 20, 2023

Conversation

timostamm
Copy link
Member

#582 and #575 and surfaced an issue with the UniversalHandler implementations for the Connect, gRPC, and gRCP-web protocols: The function returns a promise for a UniversalServerResponse that resolves immediately, before the RPC implementation is invoked.

This causes some server implementations (fetch API handlers, and createRouterTransport, but not @bufbuild/connect-node) to write the response headers before the RPC implementation had a chance to add its own response headers.

The fix is to wait until the first response body byte is produced before resolving the response.

Copy link
Contributor

@dimitropoulos dimitropoulos left a comment

Choose a reason for hiding this comment

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

#590 passing CI confirms that this works! None of my feedback is super blocking.

Copy link
Contributor

@dimitropoulos dimitropoulos left a comment

Choose a reason for hiding this comment

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

update: inspired by https://github.com/bufbuild/connect-es/pull/588/files#r1171944922 I went ahead and checked the header values the same way for the interceptor tests in 0526f9a and it doesn't pass, but I'm not sure why? Could there be a sneaky bug somewhere still, or did I just do something wrong?

@timostamm
Copy link
Member Author

I went ahead and checked the header values the same way for the interceptor tests in 0526f9a and it doesn't pass, but I'm not sure why?

Looks like you set "stream-response-header", but expect "unary-response-header".

@timostamm timostamm merged commit ca5c602 into main Apr 20, 2023
@timostamm timostamm deleted the tstamm/fix-handlers-resolve branch April 20, 2023 11:06
@dimitropoulos
Copy link
Contributor

Looks like you set "stream-response-header", but expect "unary-response-header".

oh phew! thanks!!

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