Details

Reviewers

tomek
• max
varun
• jim
atul
ashoat

Commits

rCOMMaa878e221585: [services] Backup - Reactor base classes

Summary

This lets us avoid adding boilerplate code for handling the bidirectional communication. All we have to do is define a class that contains a method named handleRequest, takes a request, and returns a response. In a case when we want to end the connection, we just have to throw EndConnectionError. This isn't really an error, just information that we'd like to disconnect.

Depends on D3245

Test Plan

cd services
yarn run-backup-service

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Harbormaster completed remote builds in B7012: Diff 9890.Feb 24 2022, 3:46 AM

• karol added inline comments.Feb 24 2022, 3:57 AM

services/backup/docker-server/contents/server/src/ReactorBase.h
29		I think we should throw an error here. The assert is a leftover from me refactoring an example code but a situation when we try to delete the object and we didn't finish the connection earlier is definitely an exception.
32 ↗	(On Diff #9802)	For me, it looks odd too, but I searched for this online and it is okay to let the object commit suicide as long as we're sure we're not going to use the object after this(that would be an undefined behavior). I think we can trust the grpc example, I'm also posting links to two comments about this: https://github.com/grpc/grpc/blob/16a3ce51ff7c308c9e8798f1a40b10d172731fab/include/grpcpp/impl/codegen/server_callback.h#L266 https://github.com/grpc/grpc/blob/16a3ce51ff7c308c9e8798f1a40b10d172731fab/include/grpcpp/impl/codegen/server_callback.h#L431

Looks ok! Just one important question inline about releasing resources in case of throwing an exception in OnDone. It would be also great it @atul and @varun could give a review.

services/backup/docker-server/contents/server/src/ReactorBase.h
24 ↗	(On Diff #9890)	I think your approach here is better, and I would probably use the same, but have you considered making `request` and `response` protected instead of passing them as arguments to this function? I guess classes that are derived from this would be less readable in that case.
44–46 ↗	(On Diff #9890)	What happens when we throw here? Is the whole instance going to stop? When throwing, we're not deleting this. Is this a memory leak?
72 ↗	(On Diff #9890)	Why are we using `gpr_log`?
32 ↗	(On Diff #9802)	Ok, thanks for doing the research! I think we should keep this approach then

This revision now requires changes to proceed.Feb 24 2022, 4:42 AM

• karol added inline comments.Feb 24 2022, 5:56 AM

services/backup/docker-server/contents/server/src/ReactorBase.h
24 ↗	(On Diff #9890)	This problem only appears in the dev mode because of the way we use the collections. and it only will occur in tests because in the service run locally we will have a delay that's going to come from the grpc connection.

• karol added inline comments.Feb 24 2022, 5:57 AM

services/backup/docker-server/contents/server/src/ReactorBase.h
24 ↗	(On Diff #9890)	sorry wrong place

I did a little research and I don't think we need to handle checking if OnDone is called after Finish:

https://github.com/grpc/grpc/blob/16a3ce51ff7c308c9e8798f1a40b10d172731fab/include/grpcpp/impl/codegen/server_callback.h

ServerCallbackReaderWriter is used in the ServerBidiReactor. It inherits from ServerCallbackCall:

https://github.com/grpc/grpc/blob/16a3ce51ff7c308c9e8798f1a40b10d172731fab/include/grpcpp/impl/codegen/server_callback.h#L79

This comment explains it all. I'm going to push these changes + getting rid of the logger use. I tested it on the sandbox and it works well.

services/backup/docker-server/contents/server/src/ReactorBase.h
24 ↗	(On Diff #9890)	Doesn't seem like a good idea for me. I think it's better to hide the fields and explicitly mark them as this method's arguments. It's just more readable.

update

Harbormaster completed remote builds in B7022: Diff 9903.Feb 24 2022, 7:59 AM

• karol planned changes to this revision.Feb 25 2022, 1:32 AM

• karol added inline comments.

services/backup/docker-server/contents/server/src/ReactorBase.h
49–53 ↗	(On Diff #9903)	I think we should return a pointer of status. Then, if we return a nullptr, it means we want to continue handling requests. Only returning a non-null status would indicate that the status is final and we want to end the connection. For now, I don't see a way of telling whether we want to continue the connection or not.

fix handling continuous connections

Harbormaster completed remote builds in B7039: Diff 9923.Feb 25 2022, 2:05 AM

tomek accepted this revision.Feb 25 2022, 3:21 AM

This revision is now accepted and ready to land.Feb 25 2022, 3:21 AM

Herald added a reviewer: ashoat. · View Herald TranscriptFeb 25 2022, 3:21 AM

Herald added a subscriber: • benschac. · View Herald Transcript

This revision now requires review to proceed.Feb 25 2022, 3:21 AM

• karol added inline comments.Feb 25 2022, 4:45 AM

services/backup/docker-server/contents/server/src/ReactorBase.h
27–30 ↗	(On Diff #9903)	this function seems redundant, we can just call `this->Finish(status)`

Let's have somebody who has experience with these "reactors" review this before I look at it

• karol planned changes to this revision.Mar 2 2022, 7:28 AM

• karol retitled this revision from [services] Backup - Reactor base to [services] Backup - Reactor base classes.

introduce reactor base classes for

read
write
bidi

I know we do not use write reactor but I'd add it anyway since we're going to need it one day, plus it will be added to the services' lib for sure and having it is not expensive.

services/backup/docker-server/contents/server/src/Reactors/ReadReactorBase.h
52 ↗	(On Diff #10019)	new line
services/backup/docker-server/contents/server/src/Reactors/WriteReactorBase.h
63 ↗	(On Diff #10019)	new line

new lines, namespaces

Harbormaster completed remote builds in B7109: Diff 10019.Mar 2 2022, 7:34 AM

Harbormaster completed remote builds in B7110: Diff 10020.Mar 2 2022, 7:37 AM

• karol retitled this revision from [services] Backup - Reactor base to [services] Backup - Reactor base classes.Mar 3 2022, 1:45 AM

I think in the bidi reactor and the write reactor the response field should be reset before every call of handleRequest and writeResponse. Doing it this way, we avoid having to delete the fields of the responses that was set for the previous calls.

reset response before every call that may modify it

Harbormaster completed remote builds in B7162: Diff 10088.Mar 4 2022, 9:46 AM

At a high level I think this looks good, added some nits inline. I have very little experience with templates in C++, so it'd be good to have someone with more experience take a look.

One thing that seems a little weird to me is the delete this; in the OnDone handler. My instinct is there must be a cleaner way to handle this, but I'm not sure what that would be. It seems like we should have some object that "owns" the reactor and handles deallocating the memory when we're done?

services/backup/docker-server/contents/server/src/Reactors/BidiReactorBase.h
11 ↗	(On Diff #10088)	I have extremely limited experience with templates but I expected to see `typename` here. Looks like `class` and `typename` are interchangeable though: There is no semantic difference between `class` and `typename` in a type-parameter-key.
40 ↗	(On Diff #10088)	Wonder if we could make this more descriptive? Maybe mention that this is coming from `OnReadDone`?
45–46 ↗	(On Diff #10088)	Should we use `make_unique` here?
48 ↗	(On Diff #10088)	I might be misunderstanding, but it looks like we're pretty much immediately dereferencing the `grpc::Status` "owned" by the `unique_ptr`? Could we do something like this? auto status = this->handleRequest(this->request, &this->response); if (status) { this->Finish(status); return; }
53 ↗	(On Diff #10088)	Think we could use braced initialization here?
services/backup/docker-server/contents/server/src/Reactors/ReadReactorBase.h
29 ↗	(On Diff #10088)	I think we should use braced initialization here
45–49 ↗	(On Diff #10088)	As with above, I think(?) we can do something like: auto status = this->handleRequest(this->request, &this->response); if (status) { this->Finish(status); return; }
51 ↗	(On Diff #10088)
services/backup/docker-server/contents/server/src/Reactors/WriteReactorBase.h
32–36 ↗	(On Diff #10088)	Nit: I think we can probably write this a bit clearer/with full sentences/etc
42–46 ↗	(On Diff #10088)	As with above, I think(?) we can do something like: auto status = this->handleRequest(this->request, &this->response); if (status) { this->Finish(status); return; }
58 ↗	(On Diff #10088)
64 ↗	(On Diff #10088)

atul added a reviewer: ashoat.Mar 4 2022, 12:16 PM

One thing that seems a little weird to me is the delete this; in the OnDone handler. My instinct is there must be a cleaner way to handle this, but I'm not sure what that would be. It seems like we should have some object that "owns" the reactor and handles deallocating the memory when we're done?

We already discussed this in this revision, please see https://phabricator.ashoat.com/D3246#87890

services/backup/docker-server/contents/server/src/Reactors/BidiReactorBase.h
11 ↗	(On Diff #10088)	Yeah, I don't think it matters that much.
40 ↗	(On Diff #10088)	reading error is fired only from `OnReadDone`. I don't mind changing it though. BTW I find this a bit weird that grpc doesn't tell us what went wrong, we just have a `bool` flag...
45–46 ↗	(On Diff #10088)	I don't think so. `make_unique` should be used when we want to create an object by passing some stuff into the constructor. `this->handleRequest` returns already created pointer so I think we're good here.
48 ↗	(On Diff #10088)	What you're suggesting is something against the whole idea here. I'm guessing you'd rather return a normal value from `handleRequest` instead of returning a pointer. The idea behind returning a pointer is that when we return a `nullptr`, we should proceed with the further requests for the current connection. The status that is eventually returned here stands for the final status of the entire communication that includes multiple requests. We could use `folly::optional` here, a pointer is just faster to implement but if you feel like optional is more readable, I can create a task for this and update it in the future diff.
53 ↗	(On Diff #10088)	In this case, we would have to refactor all occurrences of invoking the `Finish` method. In general, it doesn't look good to me. I think, whenever we have a chance for calling a constructor, we should do so, it's just more consistent and better to read.
services/backup/docker-server/contents/server/src/Reactors/ReadReactorBase.h
29 ↗	(On Diff #10088)	Well, as above. I think we should keep consistency across the codebase. In the rest of the code, we use parentheses `()` in initialization lists. So I think we'd better either stay with this or change it everywhere. BTW. is there any advantage of using brackets `{}` instead of parentheses `()`?
services/backup/docker-server/contents/server/src/Reactors/WriteReactorBase.h
32–36 ↗	(On Diff #10088)	Writing comments in the code I always try to do it as tight and informative as possible. Truth is, you can always write something better. If the comment is not super clear or is misleading, etc., feel free to suggest an edit for this.

error message

Harbormaster completed remote builds in B7176: Diff 10102.Mar 7 2022, 3:46 AM

Let's move the Reactors directory out of the backup directory so we can use it in other services too

This revision now requires changes to proceed.Mar 7 2022, 7:59 AM

Resigning for now because I have minimal context on this "Reactors" stuff, and three of you guys have a lot of context since I'm probably not needed. Feel free to re-add me at the end of the review process if there is something you want my perspective on, or something specific you think I can be helpful on

In D3246#90413, @varun wrote:

Let's move the Reactors directory out of the backup directory so we can use it in other services too

This is tracked in https://linear.app/comm/issue/ENG-324/avoid-repetitions-in-services-code

Please, review the code here, once we decide to take care of avoiding repetitions, it's going to be in a separate diff anyway and we should cover all repetitions at once.

varun accepted this revision.Mar 14 2022, 6:39 PM

varun added inline comments.

services/backup/docker-server/contents/server/src/ReactorBase.h
15 ↗	(On Diff #9802)	yeah, the reactor object should only be accessed from a single thread. work that could block (like network calls) should be sent to other threads, however, so that this one is never blocked

This revision is now accepted and ready to land.Mar 14 2022, 6:39 PM

Closed by commit rCOMMaa878e221585: [services] Backup - Reactor base classes (authored by • karol). · Explain WhyMar 21 2022, 2:28 AM

This revision was automatically updated to reflect the committed changes.

• karol added a commit: rCOMMaa878e221585: [services] Backup - Reactor base classes.

• karol mentioned this in D3469: [services] Blob - Add server base reactor classes - bidi reactor.Mar 30 2022, 6:24 AM

[services] Backup - Reactor base classes
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 9832

services/backup/docker-server/contents/server/src/ReactorBase.h

[services] Backup - Reactor base classesClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 9832

services/backup/docker-server/contents/server/src/ReactorBase.h

[services] Backup - Reactor base classes
ClosedPublic
Actions

Revision Contents
Changeset List