Page MenuHomePhabricator
Differential D3246

[services] Backup - Reactor base classes
ClosedPublic
Actions

Authored by • karol on Feb 18 2022, 5:02 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 23, 6:09 PM
Unknown Object (File)
Tue, Apr 23, 6:09 PM
Unknown Object (File)
Tue, Apr 23, 6:09 PM
Unknown Object (File)
Tue, Apr 23, 6:09 PM
Unknown Object (File)
Tue, Apr 23, 6:09 PM
Unknown Object (File)
Tue, Apr 23, 6:09 PM
Unknown Object (File)
Tue, Apr 23, 6:09 PM
Unknown Object (File)
Tue, Apr 23, 6:09 PM
View All Files
Subscribers
• adrian
ashoat
atul
• benschac
tomek

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
Lint Not Applicable
Unit
Tests Not Applicable

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 ↗(On Diff #9832)

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

tomek requested changes to this revision.Feb 24 2022, 4:42 AM

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

• karol added a comment.Feb 24 2022, 7:51 AM

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.

• karol updated this revision to Diff 9903.Feb 24 2022, 7:51 AM

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.

• karol updated this revision to Diff 9923.Feb 25 2022, 2:00 AM

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)

ashoat removed a reviewer: ashoat. ashoat added 2 blocking reviewer(s): varun, atul.Feb 27 2022, 9:07 PM

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.
• karol updated this revision to Diff 10019.Mar 2 2022, 7:28 AM
• karol retitled this revision from [services] Backup - Reactor base classes to [services] Backup - Reactor base.

introduce reactor base classes for

  • read
  • write
  • bidi
• karol planned changes to this revision.Mar 2 2022, 7:30 AM

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

• karol updated this revision to Diff 10020.Mar 2 2022, 7:32 AM

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
• karol planned changes to this revision.Mar 4 2022, 9:36 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.

• karol updated this revision to Diff 10088.Mar 4 2022, 9:40 AM

reset response before every call that may modify it

Harbormaster completed remote builds in B7162: Diff 10088.Mar 4 2022, 9:46 AM
atul accepted this revision.EditedMar 4 2022, 12:16 PM

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
• karol planned changes to this revision.Mar 7 2022, 3:40 AM

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.

• karol updated this revision to Diff 10102.Mar 7 2022, 3:41 AM

error message

Harbormaster completed remote builds in B7176: Diff 10102.Mar 7 2022, 3:46 AM
varun requested changes to this revision.Mar 7 2022, 7:59 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
ashoat resigned from this revision.Mar 7 2022, 10:45 PM

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

• karol requested review of this revision.Mar 14 2022, 9:34 AM
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

Revision Contents
Changeset List

PathSize
services/
backup/
docker-server/
contents/
server/
1 line
src/
Reactors/
68 lines
59 lines
69 lines

Diff 10534

View Options

services/backup/docker-server/contents/server/CMakeLists.txt

Loading...
View Options

services/backup/docker-server/contents/server/src/Reactors/BidiReactorBase.h

Loading...
View Options

services/backup/docker-server/contents/server/src/Reactors/ReadReactorBase.h

Loading...
View Options

services/backup/docker-server/contents/server/src/Reactors/WriteReactorBase.h

Loading...