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 ↗	(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

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		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		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		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	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	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	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

Diff 9890

View Options

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

This file was added.

				// @flow

				import type { Account as OlmAccount } from '@commapp/olm';

				import { ServerError } from 'lib/utils/errors.js';

				import { dbQuery, SQL } from '../database/database.js';
				import { fetchCallUpdateOlmAccount } from '../updaters/olm-account-updater.js';
				import { createPickledOlmSession } from '../utils/olm-utils.js';

				async function createOlmSession(
				initialEncryptedMessage: string,
				olmSessionType: 'primary' \| 'notifications',
				cookieID: string,
				): Promise<void> {
				const isPrimary = olmSessionType === 'primary';

				let account;
				const accountCallback = async (callbackAccount: OlmAccount) => {
				account = callbackAccount;
				};
				await fetchCallUpdateOlmAccount(olmSessionType, accountCallback);
				ashoatUnsubmitted Not Done Inline Actions Do we need to use this function given we're just doing a read? I think the function call is only necessary if we need to lock for writes. Instead of calling this function and then separately querying for the `pickling_key` below, I think it would be better to replace both with a single SQL query that fetches both the account and the pickling key ashoat: Do we need to use this function given we're just doing a read? I think the function call is…
				marcinAuthorUnsubmitted Done Inline Actions Do we need to use this function given we're just doing a read? I think the function call is only necessary if we need to lock for writes. We need since if someone if performing a write operation we need to wait with reading until they are done. marcin: > Do we need to use this function given we're just doing a read? I think the function call is…
				ashoatUnsubmitted Not Done Inline Actions Why do we need to wait for the write to complete? Is there something bad that can happen in this scenario, that wouldn't happen in a scenario where `createOlmSession` got called right before an update? I assume we (and Olm) need to be resistant to the second scenario, so my assumption is that the first scenario will work as well ashoat: Why do we need to wait for the write to complete? Is there something bad that can happen in…
				marcinAuthorUnsubmitted Done Inline Actions We need to answer some questions here: Does creating a new session using an account changes internal state of an account - does it change its pickled content we keep in the db. If the answer is no, then we can do unblocking read and skip updating database. If the answer to the first question is yes, then we need to answer another question. Is olm able to recover if we skip persisting updated olm account in the db after creating a session. If the answer to this question is yes, we can do unblocking read and skip updating database. If the answer to the second question is no, then we have to ensure the sequence `read -> create session-> update db` executes atomically. Imagine a case when we read olm account with prekey older than 30 days and use it the to create olm session. When we are busy creating olm session the cron generates new prekey and persists to the db an account with new prekey. Once we are done creating new olm session we persist to the db an account with the old prekey and we wait another 24 hours before we create new one. This is just an example but I hope it presents my point well. cc @anunay marcin: We need to answer some questions here: 1. Does creating a new session using an account changes…
				ashoatUnsubmitted Not Done Inline Actions I think no. It may be worth confirming this... perhaps the easiest way would be to modify @anunay's unit tests in D7670 to create a new session, and then to check to see if the contents of the pickled account have changed. N/A I am not sure I follow your concern about this scenario. It should be okay for the native/web client to create an outgoing session using the keyserver's old prekey, since the keyserver will keep the old prekey for 24 hours. I'm not sure what you mean when you say "Once we are done creating new olm session we persist to the db an account with the old prekey"... the `olm-session-creator.js` code you introduce here does not seem to save accounts (just sessions). As for the cronjob, it will save the new account, but the old prekey will still be there, so it should not cause any problems. ashoat: 1. I think no. It may be worth confirming this... perhaps the easiest way would be to modify…
				marcinAuthorUnsubmitted Done Inline Actions the olm-session-creator.js code you introduce here does not seem to save accounts (just sessions) It does. `fetchCallUpdate` saves account after calling lambda. Scenario with the prekey was just an example. My point is that if creating new olm session alters pickled content of an account then we must ensure atomicity in sequence `read->create session -> write`. Otherwise changes we made when creating session might be lost when something else writes. If there are no changes when creating a session, or loosing those changes can be handled by olm, then we can bypass `fetchCallUpdate` function and just read relevant information from the db. However if we can't loose those changes we need atomicity and `fetchCallUpdate` guarantees that. marcin: > the olm-session-creator.js code you introduce here does not seem to save accounts (just…
				ashoatUnsubmitted Not Done Inline Actions the olm-session-creator.js code you introduce here does not seem to save accounts (just sessions) It does. `fetchCallUpdate` saves account after calling lambda. My whole point is that `fetchCallUpdateOlmAccount` does not need to be called. If you take my solution, this will not be a problem. Scenario with the prekey was just an example. My point is that if creating new olm session alters pickled content of an account then we must ensure atomicity in sequence `read->create session -> write`. Otherwise changes we made when creating session might be lost when something else writes. If there are no changes when creating a session, or loosing those changes can be handled by olm, then we can bypass `fetchCallUpdate` function and just read relevant information from the db. However if we can't loose those changes we need atomicity and `fetchCallUpdate` guarantees that. I agree with this. I am pretty sure that creating a new Olm session does not alter pickled content of the account. As suggested in my previous comment, you can confirm this by testing. ashoat: > > the olm-session-creator.js code you introduce here does not seem to save accounts (just…
				marcinAuthorUnsubmitted Done Inline Actions Confirmed by testing that session creation does not alter pickled content of olm account. marcin: Confirmed by testing that session creation does not alter pickled content of olm account.
				if (!account) {
				throw new ServerError('missing_olm_account');
				}

				const [picklingKeyResult] = await dbQuery(
				SQL`
				SELECT pickling_key
				FROM keyserver_olm_accounts
				ashoatUnsubmitted Not Done Inline Actions In an earlier diff I requested a name change of this table. I think updating this sooner rather than later will save you some refactoring work ashoat: In an earlier diff I requested a name change of this table. I think updating this sooner rather…
				WHERE is_primary = ${isPrimary}
				`,
				);
				if (picklingKeyResult.length === 0) {
				throw new ServerError('missing_account_pickling_key');
				}
				const picklingKey = picklingKeyResult[0].pickling_key;

				let pickledOlmSession;
				try {
				pickledOlmSession = await createPickledOlmSession(
				account,
				picklingKey,
				initialEncryptedMessage,
				);
				} catch (e) {
				throw new ServerError('olm_session_creation_failure');
				}

				// We match the native client behavior here where olm session is overwritten
				// in case it is initialized twice for the same pair of identities
				await dbQuery(
				SQL`
				INSERT INTO olm_sessions (cookie_id, is_primary, pickled_olm_session)
				VALUES (${cookieID}, ${isPrimary}, ${pickledOlmSession})
				ON DUPLICATE KEY UPDATE
				pickled_olm_session = ${pickledOlmSession}
				`,
				);
				}

				export { createOlmSession };

[services] Backup - Reactor base classes
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 9890

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

[services] Backup - Reactor base classesClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 9890

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

[services] Backup - Reactor base classes
ClosedPublic
Actions

Revision Contents
Changeset List