Details

Reviewers

tomek
• karol
atul

Commits

rCOMMc1469c21a1aa: [native] register_user client method

Summary

This method handles the bidi streaming interaction with the Identity service for registering a user. Some of the logic has been moved to subroutines that can be found earlier in the stack. I tested this method by creating a main function and calling it, and successfully receiving an access token. I tested the error cases by modifying the Identity service (running locally) to simulate various siutations (wrong response returned, server hangup, invalid response bytes) and checked that they were handled correctly on the client side by inspecting the logs.

Depends on D4857

Test Plan

See the summary above

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

varun created this revision.Aug 10 2022, 4:43 PM

Herald added subscribers: • abosh, atul, • adrian, ashoat. · View Herald TranscriptAug 10 2022, 4:43 PM

Harbormaster completed remote builds in B11288: Diff 15533.Aug 10 2022, 4:51 PM

varun requested review of this revision.Aug 10 2022, 4:51 PM

This function is really long and complicated. Could you help a bit to make it easier to review? Extracting some function was helpful, but can we split this function even more? Also, it would be really useful to see some comments explaining the steps that are happening. Thanks!

native/cpp/CommonCpp/grpc/grpc_client/src/lib.rs
106 ↗	(On Diff #15533)	I've got really confused by this line, because `std::sync::mpsc::channel` doesn't take arguments. But in fact, we're using `tokio::sync::mpsc::channel`. It's not a strong opinion, but for this one I would rather use `tokio::sync::mpsc::channel` explicitly.

This revision now requires changes to proceed.Aug 11 2022, 4:47 AM

Add comments, move some logic to the subroutines

varun edited parent revisions, added: D4814: [native] another helper for PAKE registration on client side; removed: D4801: [native] helper for pake login on client side.Aug 11 2022, 12:04 PM

In D4802#138731, @tomek wrote:

This function is really long and complicated. Could you help a bit to make it easier to review? Extracting some function was helpful, but can we split this function even more? Also, it would be really useful to see some comments explaining the steps that are happening. Thanks!

Ah wait, did you mean like inline comments on phabricator? If so, I'll remove the code comments

Harbormaster completed remote builds in B11321: Diff 15578.Aug 11 2022, 12:14 PM

varun added a child revision: D4815: [native] add blocking version of get_user_id method.Aug 11 2022, 12:30 PM

In D4802#139031, @varun wrote:

In D4802#138731, @tomek wrote:

This function is really long and complicated. Could you help a bit to make it easier to review? Extracting some function was helpful, but can we split this function even more? Also, it would be really useful to see some comments explaining the steps that are happening. Thanks!

Ah wait, did you mean like inline comments on phabricator? If so, I'll remove the code comments

I wasn't sure whether inline or code comments are better so I left it unspecified. I think now that having these in code is better because it might help when reading the code in the future, so let's keep them.

Thanks for adding the comments and refactoring the code! I think it is more readable now, but there are a couple of other thing we can try to improve it further.
One possible improvement is to change the approach and instead of having a loop introduce a separate function for each step. Then we would call them one by one and verify the state, something like:

let message = response.message().await?;
handle_registration(message, ...);
let message = response.message().await?;
handle_login(message, ...);
...

This approach has a couple of advantages: we get rid of additional logic (while and num_messages_received), each step is separated into a function, etc. There are some disadvantages as well, but wanted to get some thoughts on it.

In general, having a match that spans multiple lines for each arm is really bad for readability. We should think about the ways to avoid this pattern.

native/cpp/CommonCpp/grpc/grpc_client/src/lib.rs
124–129 ↗	(On Diff #15578)	It is not strictly related to this diff, but we've introduced a couple of functions that take a lot of parameters. Maybe we can create some structs to hold these, so that functions' api is simplified?
142–147 ↗	(On Diff #15578)	What do you think about `matching message.data` and `num_messages_received` at the same time?
165–168 ↗	(On Diff #15578)	I think the readability would be improved if we used a struct instead of tuple. When reading the code it is hard to know what is the meaning of `0` and `1`.

This revision now requires changes to proceed.Aug 12 2022, 3:47 AM

I could not apply this diff locally, could you please fix this? I tried to apply this after the 2 previous diffs from this stack.

I also think that splitting this diff would be better for reviewing.

@karol, when trying to arc patch did you run the Git command to pull in all external tags? (We should probably document this somewhere under "Working With Phabricator"...)

address feedback

In D4802#139322, @karol wrote:

I could not apply this diff locally, could you please fix this? I tried to apply this after the 2 previous diffs from this stack.

I also think that splitting this diff would be better for reviewing.

It works for me, I think you need to pull the tags, like ashoat said

In D4802#139287, @tomek wrote:
In D4802#139031, @varun wrote:

In D4802#138731, @tomek wrote:

This function is really long and complicated. Could you help a bit to make it easier to review? Extracting some function was helpful, but can we split this function even more? Also, it would be really useful to see some comments explaining the steps that are happening. Thanks!

Ah wait, did you mean like inline comments on phabricator? If so, I'll remove the code comments

I wasn't sure whether inline or code comments are better so I left it unspecified. I think now that having these in code is better because it might help when reading the code in the future, so let's keep them.

Thanks for adding the comments and refactoring the code! I think it is more readable now, but there are a couple of other thing we can try to improve it further.
One possible improvement is to change the approach and instead of having a loop introduce a separate function for each step. Then we would call them one by one and verify the state, something like:
let message = response.message().await?;
handle_registration(message, ...);
let message = response.message().await?;
handle_login(message, ...);
...
This approach has a couple of advantages: we get rid of additional logic (while and num_messages_received), each step is separated into a function, etc. There are some disadvantages as well, but wanted to get some thoughts on it.

In general, having a match that spans multiple lines for each arm is really bad for readability. We should think about the ways to avoid this pattern.

Your feedback made a lot of sense. I've taken your suggestion and removed the loop.

native/cpp/CommonCpp/grpc/grpc_client/src/lib.rs
124–129 ↗	(On Diff #15578)	https://linear.app/comm/issue/ENG-1629/refactor-rust-functions-to-take-fewer-params I'll address this feedback later, but created the above linear task to track because I agree we should make these functions simpler to use
142–147 ↗	(On Diff #15578)	`num_messages_received` is gone in the latest revision
165–168 ↗	(On Diff #15578)	changed the helper function to return a single struct