Skip to content

Conversation

@sleipnir
Copy link
Collaborator

This PR corrects a pattern matching issue in the send_response function and adds integration tests for the map_error function. There was a suspicion that this function was not correctly sending errors in the trailers, so I decided to add new tests to specifically validate this.

@aseigo I've brought the changes to the master branch in this PR, and I see that everything works. I haven't been able to reproduce your map_error error aside from the pattern matching issue. The tests are going according to what the Elixir customer expects.

Is there another way I can reproduce the problem?

Captura de tela de 2025-12-16 12-39-49 Captura de tela de 2025-12-16 12-41-09

@aseigo
Copy link
Contributor

aseigo commented Dec 16, 2025

Some testing later ... I can confirm that clients are receiving the error. I can see the error in gprcurl/grpcui. Output from grpcurl:

Resolved method descriptor:
rpc find ( .Redacted.GRPC.Discovery.ServicesRequest ) returns ( .Redacted.GRPC.Discovery.ServicesReply );

Request metadata to send:
(empty)

Response headers received:
(empty)

Response trailers received:
content-length: 0
content-type: application/grpc+proto
date: Tue, 16 Dec 2025 16:32:51 GMT
server: Cowboy
Sent 1 request and received 0 responses
ERROR:
  Code: NotFound
  Message: Do you see me?

and the same in the UI:

Screenshot_20251216_173507

So the error gets through in the response data, but there are no grpc trailers sent. This breaks with web clients in which grpcweb requires the grpc trailers to be sent encoded within the response data.

So this gets us most of the way there, which is great news. What's left is ensuring that the grpc trailers are also sent ... thoughts?

@aseigo
Copy link
Contributor

aseigo commented Dec 16, 2025

I see why it doesn't get into the grpcweb trailers. When an exception is thrown, then Cowboy.Handler.send_error gets called, which itself calls send_error_trailers, and send_error_trailers calls :cowboy_req.reply(status, trailers, req) directly while my branch with grpcweb trailers only changes Cowboy.Handler.info for {:stream_trailers, trailers} messages ... so the error path never makes it into the grpcweb client.

If this branch gets merged in, I will adjust the grpcweb trailers branch to work with these changes, and then we should be OK

@sleipnir
Copy link
Collaborator Author

I see why it doesn't get into the grpcweb trailers. When an exception is thrown, then Cowboy.Handler.send_error gets called, which itself calls send_error_trailers, and send_error_trailers calls :cowboy_req.reply(status, trailers, req) directly while my branch with grpcweb trailers only changes Cowboy.Handler.info for {:stream_trailers, trailers} messages ... so the error path never makes it into the grpcweb client.

Cool

If this branch gets merged in, I will adjust the grpcweb trailers branch to work with these changes, and then we should be OK

Your PR regarding the trailers has already been approved and will be added to the master as soon as I can find the best way to merge them. I'll probably try merging your first one and incorporating the changes into the other PR for the adapter, but I'm still working on it.

@aseigo
Copy link
Contributor

aseigo commented Dec 16, 2025

With a very simple (~10 lines) change to the grpcweb branch rebased onto this branch, we now see errors on both native and web clients with returning {:error, ...} tuples from map or map_error. Very nice!

I suggest merging this branch, and I can then close #486 and ammend #481

@sleipnir sleipnir merged commit 2060e05 into master Dec 16, 2025
6 checks passed
@sleipnir sleipnir deleted the test/stream-map-error branch December 16, 2025 17:19
@sleipnir
Copy link
Collaborator Author

With a very simple (~10 lines) change to the grpcweb branch rebased onto this branch, we now see errors on both native and web clients with returning {:error, ...} tuples from map or map_error. Very nice!

I suggest merging this branch, and I can then close #486 and ammend #481

Done!

@sleipnir
Copy link
Collaborator Author

@aseigo Thank you for your patience and continued contribution.

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.

3 participants