GrpcWebClientReadableStream: keep falsy data #1230
Conversation
|
added |
|
there are more changes needed in |
|
Hi! Thanks for the contribution and sorry for the delay! 2 quick questions:
thanks! |
|
Hi, thanks for helping out with this. How to use a custom codec:We haven't been using .proto files to define the methods, so we construct a bunch of our own Adding a testYa let me try adding one |
Oh aha! Intersting! Thanks for the info! I guess that's one way of using the library (but without the codegen) 😃 I wonder if we mention the use of library in this way on any documentation? Or this is just one creative way you found out about using it? (I'm guess in the latter case I'm not sure if such use will be "officially supported" going forward, even if we could probably fix a few non-harming edge-cases right now) 😃
thanks!! |
|
Ah thanks for the info! I did a quick search and found that the majority of mentioning for custom codec is for golang. (there are some mentions on other languages but nothing seems officially documented.) So personally i'd still consider this a borderline edge use case :) Happy to take small patches if it's easy to maintain, but probably no commitment for major feature support going forward :) |
|
aaah I just saw the new release and realized this was still pending
we're happy with that too. if you could please review 🙏 |
aha right i've forgot as well :)
thanks for your interest here! On a 2nd thought, I'll need to check with the team on our stance on custom codec before getting back at you. Thanks! |
|
Sounds good, thanks for checking with the team |
|
Of course! Oh and actually, since you'll be depending on But we could explore our options there too.. In the meanwhile, maybe you could rebase your code onto 1.4.0 and see if it breaks you? Thanks! |
|
interesting, thanks for the heads up. let me experiment with the changes in 1.4.0. if it's only from the typescript headers, we should be fine |
|
update: yes, it does break our |
aha interesting thanks for confirming! So i guess that would work but is somewhat a "hack".. :) |
| } else { | ||
| unaryMsg = response; |
There was a problem hiding this comment.
I'm not sure we should set the unaryMsg on an else {} case — it seems a bit unsafe to set it before checking nullability (even though it might be ok based on existing callers).
Actually.. I'm curious what if you don't make this change? With the existing code, wouldn't the stream.on('end' ...) below eventually trigger a callback(null, null) call which will fall into this else {}clause, which will trigger the resolve(...) call?
There was a problem hiding this comment.
I'm not sure we should set the
unaryMsgon anelse {}case — it seems a bit unsafe to set it before checking nullability (even though it might be ok based on existing callers).
This single callback has to do 5 different jobs in a function.
- receive an error
- receive the response value
- receive the status
- receive the metadata
- be informed that we have all pieces needed to give the caller the unary response
So far, it has done this by using a giant if ... tree and testing for various arguments' falsiness. Supporting falsy response values would require a way to differentiate job 2 and job 5. This version declares that job 5 has trigger = true, while job 2 has trigger = false, which overall sticks to the existing theme of "one of the args is truthy, just check which one."
As for how can we ensure it's safe: setCallback_ is private and the callback handle is kept private, so we have control over what will ever call it, and we can make sure all callers follow this new protocol in this PR.
Actually.. I'm curious what if you don't make this change? With the existing code, wouldn't the
stream.on('end' ...)below eventually trigger acallback(null, null)call which will fall into thiselse {}clause, which will trigger theresolve(...)call?
If we don't change the protocol:
- A falsy response never gets assigned into
unaryMsg. I believe our use case is sloppy-tolerant enough not to experience any problems if grpc-web givesundefinedinstead of0or""orfalseornull, but giving the right type is still nice. - The
resolvewould be called twice. I think standards compliant Promise implementations are specified to ignore extra calls, so that shouldn't break anything. But that's a relatively more obscure detail, and I think the code is easier for beginners to verify if it only ever callsresolveonce.
There was a problem hiding this comment.
- Actually I don't know about the order of calls to this callback. And if I did know the order, I don't know it's an order that gRPC intends to stick to. If job 2 comes before job 4, for example (I vaguely recall seeing something about trailers in gPRC? Maybe grpc-web doesn't support that though.), we'd incorrectly
resolvebefore setting the unary metadata.
There was a problem hiding this comment.
Hi! Thanks for the quick reply and sorry for the slow reply.. It took me a while to wrap my head around the code here.. 😅
As for how can we ensure it's safe: setCallback_ is private and the callback handle is kept private, so we have control over what will ever call it, and we can make sure all callers follow this new protocol in this PR.
What you said makes a lot of sense.. I guess i was kind hoping that there's an explicit signal for setting the response (for better robustness).. but given that the response is falsy here, what you have here is probably the best we can do for now :) (Especially given that the "callback" is not exactly "internal" since we reuse the same function for external callers (here) too..)
If we don't change the protocol:
- A falsy response never gets assigned into unaryMsg. I believe our use case is sloppy-tolerant enough not to experience any problems if grpc-web gives undefined instead of 0 or "" or false or null, but giving the right type is still nice.
- The resolve would be called twice. I think standards compliant Promise implementations are specified to ignore extra calls, so that shouldn't break anything. But that's a relatively more obscure detail, and I think the code is easier for beginners to verify if it only ever calls resolve once.
Thanks so much for explaining! This makes total sense.. :)
3, Actually I don't know about the order of calls to this callback. And if I did know the order, I don't know it's an order that gRPC intends to stick to. If job 2 comes before job 4, for example (I vaguely recall seeing something about trailers in gPRC? Maybe grpc-web doesn't support that though.), we'd incorrectly resolve before setting the unary metadata.
Yeahh i think grpc-web is not using HTTP trailers due to API limitations (see here) so what you've mentioned shouldn't happen. But thanks for the careful considerations :)
There was a problem hiding this comment.
here's something we could do, let me know if this is anymore idiomatic:
we still add an extra parameter, but instead of true = "done" and false = "get the response value," we can have true = "get the response value" and false = "done"
There was a problem hiding this comment.
pushed a version where instead of a having a unaryEndOfStream flag, there's a unaryResponseReceived flag. i.e., now "there's an explicit signal for setting the response."
added doc comment for the useUnaryResponse parameter to setCallback_
| @@ -123,18 +123,18 @@ class GrpcWebClientBase { | |||
| let unaryStatus; | |||
| let unaryMsg; | |||
| GrpcWebClientBase.setCallback_( | |||
| stream, (error, response, status, metadata) => { | |||
| stream, (error, response, status, metadata, trigger) => { | |||
There was a problem hiding this comment.
could you rename trigger to a more explicit name?
Maybe unaryEndOfStream?
| @@ -75,6 +75,33 @@ testSuite({ | |||
| assertElementsEquals(DEFAULT_UNARY_HEADER_VALUES, Object.values(headers)); | |||
| }, | |||
|
|
|||
| async testRpcFalsyResponse() { | |||
There was a problem hiding this comment.
Thanks for adding the test :)
I wonder if you could rename this testRpcFalsyResponse_ForNonProtobufDescriptor so it's more clear that this isn't part of our typical protobuf flow?
| } else { | ||
| unaryMsg = response; |
There was a problem hiding this comment.
Hi! Thanks for the quick reply and sorry for the slow reply.. It took me a while to wrap my head around the code here.. 😅
As for how can we ensure it's safe: setCallback_ is private and the callback handle is kept private, so we have control over what will ever call it, and we can make sure all callers follow this new protocol in this PR.
What you said makes a lot of sense.. I guess i was kind hoping that there's an explicit signal for setting the response (for better robustness).. but given that the response is falsy here, what you have here is probably the best we can do for now :) (Especially given that the "callback" is not exactly "internal" since we reuse the same function for external callers (here) too..)
If we don't change the protocol:
- A falsy response never gets assigned into unaryMsg. I believe our use case is sloppy-tolerant enough not to experience any problems if grpc-web gives undefined instead of 0 or "" or false or null, but giving the right type is still nice.
- The resolve would be called twice. I think standards compliant Promise implementations are specified to ignore extra calls, so that shouldn't break anything. But that's a relatively more obscure detail, and I think the code is easier for beginners to verify if it only ever calls resolve once.
Thanks so much for explaining! This makes total sense.. :)
3, Actually I don't know about the order of calls to this callback. And if I did know the order, I don't know it's an order that gRPC intends to stick to. If job 2 comes before job 4, for example (I vaguely recall seeing something about trailers in gPRC? Maybe grpc-web doesn't support that though.), we'd incorrectly resolve before setting the unary metadata.
Yeahh i think grpc-web is not using HTTP trailers due to API limitations (see here) so what you've mentioned shouldn't happen. But thanks for the careful considerations :)
| }); | ||
|
|
||
| const response = await new Promise((resolve, reject) => { | ||
| client.rpcCall( |
There was a problem hiding this comment.
Actually, after scratching my head a bit i realized that the main discussion we had was around "thenableCall" but it's not covered in tests here..
If it's not too difficult, i wonder if you could help add 1-2 thenableCall tests to cover the basic cases (e.g. normal response + false response)? Thanks a lot!!
There was a problem hiding this comment.
wow thanks so much for the new tests. Looks awesome! 😃
|
I'm getting this when I try to run the tests
is it erroneously using my systemwide chrome rather than its own version for testing? |
Aha not sure.. i forgot how i setup my chrome driver.. maybe 93 is a bit too old i can update it soon.. (although i'm using Although, if you run the test using Could you try that and see if it works? Thanks :) |
| * @param {boolean} useUnaryResponse Pass true to have the client make | ||
| * multiple calls to the callback, using (error, response, status, | ||
| * metadata, unaryResponseReceived) arguments. One of error, status, | ||
| * metadata, or unaryResponseReceived will be truthy to indicate which piece | ||
| * of information the client is providing in that call. After the stream | ||
| * ends, it will call the callback an additional time with all falsy | ||
| * arguments. Pass false to have the client make one call to the callback | ||
| * using (error, response) arguments. |
There was a problem hiding this comment.
nice comments.. appreciate it.. 😃
| @@ -272,11 +279,11 @@ class GrpcWebClientBase { | |||
| message: 'Incomplete response', | |||
| }); | |||
| } else { | |||
| callback(null, responseReceived); | |||
| callback(null, responseReceived, null, null, /* unaryResponseReceived= */ true); | |||
There was a problem hiding this comment.
this is where things get a little bit weird.. since in rpcCall we're directly taking a user-provided callback.. so this implementation is a bit "leaky".. although, probably (hopefully) not a big deal.. 😄
There was a problem hiding this comment.
oh good point. I'll open a follow up not to pass the extra flag when useUnaryResponse is off
There was a problem hiding this comment.
ah! thanks very much for the follow-up fix! 😃
| }); | ||
|
|
||
| const response = await new Promise((resolve, reject) => { | ||
| client.rpcCall( |
There was a problem hiding this comment.
wow thanks so much for the new tests. Looks awesome! 😃
| if (error) { | ||
| reject(error); | ||
| } else if (response) { | ||
| } else if (unaryResponseReceived) { |
There was a problem hiding this comment.
love this change.. looks much cleaner! Thanks!
|
I will cut a new release soon (prob. later this week) with this change. Thanks again! 😃 |
|
FYI a new release with this change is cut here :) https://github.com/grpc/grpc-web/releases/tag/1.4.2 |
cross reference #1025
When using a different codec where primitive values are allowed (we use CBOR), the current implementation omits calling the
datalisteners on falsy values, such as0andnull. But we'd like to have them.Is there anything else that relies on the current behavior that omits these values?
Discussion of changes in
GrpcWebClientBase:Current implementation uses
setCallback_in a special "useUnaryResponse" mode. In this mode, additional calls are made to the callback withstatusandmetadataparameters, then a final time with botherrorandresponseset tonull.In order to support falsy and even null message, this changes the final callback call to pass
trueto a new explicittriggerparameter.callback(null, null)now means "no error, and the response was null" (and similar for other falsy responses).setCallback_is private, and the only public wrapper does not allow the "useUnaryResponse" mode, so it should be safe to make this change.