Skip to content

Conversation

sampajano
Copy link
Collaborator

Discussed in following threads:

#1391
#1322

@sampajano sampajano enabled auto-merge (squash) February 9, 2024 02:09
@sampajano sampajano disabled auto-merge February 9, 2024 02:09
@sampajano sampajano merged commit 55b9218 into grpc:master Feb 9, 2024
@sampajano sampajano deleted the 1.doc branch February 9, 2024 02:09
@@ -13,7 +13,7 @@ frameworks for languages such as Python, Java, and Node. For details, see the
## Streaming Support
gRPC-web currently supports 2 RPC modes:
- Unary RPCs ([example](#make-a-unary-rpc-call))
- Server-side Streaming RPCs ([example](#server-side-streaming)) (NOTE: Only when [`grpcwebtext`](#wire-format-mode) mode is used.)
- Server-side Streaming RPCs ([example](#server-side-streaming)) (NOTE: Only when [`grpcwebtext`](#wire-format-mode) mode is used, or when [Nginx](#ecosystem) is used)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a surprising / confusing note. Why does Envoy, the gRPC-web recommended proxy, not support server-side streaming in binary, if Nginx is able to?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, i can agree.. I was just stating the information i gathered, but not trying to reason if it's "expected" or not.. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

The phrasing here is still confusing, even if it's true - it says only the text format supports streaming, but also that Nginx actually supports streaming regardless of format. If true it would be clearer to say that streaming conceptually works with both formats but the Envoy implementation specifically only supports streaming with the text format.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah i agree with you.. i didn't call out Envoy specifically because it's not the only proxy that supports grpc web.

Let's get a conclusion on the other comment you have, and then i'll update this text.

@@ -337,15 +337,16 @@ Multiple proxies support the gRPC-web protocol.
$ docker-compose up -d node-server envoy commonjs-client
```

2. You can also try the [gRPC-web Go proxy][].
2. [Nginx](https://www.nginx.com/) has a grpc-web module ([doc](https://nginx.org/en/docs/http/ngx_http_grpc_module.html), [announcement](https://www.nginx.com/blog/nginx-1-13-10-grpc/)). It seems to work with simple configs, according to [user feedbacks](https://github.com/grpc/grpc-web/discussions/1322)), even with server-streaming support ([user feedback](https://github.com/grpc/grpc-web/issues/1391)).
Copy link
Contributor

Choose a reason for hiding this comment

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

Neither of these nginx pages mention gRPC-web - are we sure they are referring to gRPC-web and not gRPC?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, you're right.. i think that's a very good point.. Somehow i thought they were — I probably misread initially. Maybe they are not..

In any case, there are 2 users reporting grpc-web is working, including the OP and this confirmation, and hence i changed the doc.

Maybe i should wait for one extra confirmation on the binary streaming case, but the OP sounded pretty sure in #1391 so i trusted his judgements :)

Copy link
Contributor

@dimo414 dimo414 Feb 23, 2024

Choose a reason for hiding this comment

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

I'm pretty suspicious these threads are not consistently distinguishing gRPC traffic from gRPC-web. For example application/grpc is clearly not gRPC-web. Furthermore, it seems unlikely that Nginx has somehow managed to enable streaming support for the gRPC-web binary format when your understanding was the binary format is incapable of supporting server-side streaming.

More to the point, it's impossible for Nginx to have enabled streaming support for the gRPC-web binary format without collaborating with the gRPC-web project, because the protocol spec directly says this is not supported. So if they have implemented some sort of streaming support it's not to spec.

But my bet (especially given the lack of any mention of gRPC-web in their documentation) is Nginx supports gRPC, and people are conflating it with gRPC-web because it's a "web" server that's able to proxy gRPC traffic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm pretty suspicious these threads are not consistently distinguishing gRPC traffic from gRPC-web. For example application/grpc is clearly not gRPC-web.

Yeah i can see your suspicion. I don't deny them.

Furthermore, it seems unlikely that Nginx has somehow managed to enable streaming support for the gRPC-web binary format when your understanding was the binary format is incapable of supporting server-side streaming.

This is a very good point.. thanks for pointing it out :)

More to the point, it's impossible for Nginx to have enabled streaming support for the gRPC-web binary format without collaborating with the gRPC-web project, because the protocol spec directly says this is not supported. So if they have implemented some sort of streaming support it's not to spec.

Yes another very good point.

But my bet (especially given the lack of any mention of gRPC-web in their documentation) is Nginx supports gRPC, and people are conflating it with gRPC-web because it's a "web" server that's able to proxy gRPC traffic.

Yes it's definitely a possibility. Let me loop in the original commenters.


@jmzwcn @zhumazhenis Hi! Thanks for previously participating in the discussion on nginx support.

There's a question now on the validity of the conclusions as above.

Could you help responding to it if there are some misunderstandings?

Thanks in advance!

Copy link
Contributor

Choose a reason for hiding this comment

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

@sampajano any objection to reverting these notes? At best they're unclear and IMO it would be better to remove them and hash out exactly what is known before re-adding them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK! Since the original commenters are not active at this point, i guess we can revert until upon further discussions :)

Do you want to create a PR to revert this one? Or if you prefer i can do it too.

Thanks! :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, just cut #1408

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