Page MenuHomePhabricator
Differential D3664

[services] Backup/Blob - Make sure Finish is only called once
ClosedPublic
Actions

Authored by • karol on Apr 8 2022, 2:33 AM.
Tags
None
Referenced Files
F3377221: D3664.diff
Wed, Nov 27, 4:30 AM
Unknown Object (File)
Sun, Nov 24, 11:53 AM
Unknown Object (File)
Tue, Nov 19, 1:21 PM
Unknown Object (File)
Wed, Nov 6, 11:40 AM
Unknown Object (File)
Tue, Nov 5, 5:28 PM
Unknown Object (File)
Thu, Oct 31, 8:36 PM
Unknown Object (File)
Thu, Oct 31, 8:30 PM
Unknown Object (File)
Thu, Oct 31, 8:29 PM
View All Files
Subscribers
• abosh
• adrian
ashoat
atul
• benschac
tomek

Details

Reviewers
tomek
• max
ashoat
Commits
rCOMM61665dd80d82: [services] Backup/Blob - Make sure Finish is only called once
Summary

Depends on D3636

We should make 100% sure that Finish (the method provided by gRPC) is called no more than once. Otherwise, it's going to crash.

Test Plan
cd services
yarn run-blob-service
yarn run-backup-service

Diff Detail

Repository
rCOMM Comm
Branch
backup-blob
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• karol created this revision.Apr 8 2022, 2:33 AM
Herald added subscribers: • abosh, • benschac, atul and 3 others. · View Herald TranscriptApr 8 2022, 2:33 AM
• karol edited the summary of this revision. (Show Details)Apr 8 2022, 2:35 AM
• karol edited the test plan for this revision. (Show Details)
• karol added reviewers: tomek, • max.
• karol added a parent revision: D3636: [services] Backup - pull backup reactor - logs - write response.
Harbormaster completed remote builds in B8013: Diff 11218.Apr 8 2022, 2:48 AM
• karol requested review of this revision.Apr 8 2022, 2:48 AM
ashoat added inline comments.Apr 10 2022, 7:01 PM
services/backup/docker-server/contents/server/src/Reactors/server/base-reactors/ServerBidiReactorBase.h
70 ↗(On Diff #11218)

this->finished

79 ↗(On Diff #11218)

this->finished

services/backup/docker-server/contents/server/src/Reactors/server/base-reactors/ServerReadReactorBase.h
43 ↗(On Diff #11218)

this->finished

47 ↗(On Diff #11218)

this->finished

ashoat requested changes to this revision.Apr 10 2022, 7:01 PM

(I only mentioned four locations but there are more)

This revision now requires changes to proceed.Apr 10 2022, 7:01 PM
• karol updated this revision to Diff 11336.Apr 12 2022, 1:27 AM

add this->

Harbormaster completed remote builds in B8109: Diff 11336.Apr 12 2022, 1:35 AM
ashoat accepted this revision.Apr 12 2022, 11:30 AM
ashoat added 1 blocking reviewer(s): tomek.

Don't have context on the rest, but looks good to me

tomek accepted this revision.Apr 13 2022, 2:48 AM

Are we sure that this is properly synchronized and we don't need e.g. atomic?

This revision is now accepted and ready to land.Apr 13 2022, 2:48 AM
• karol mentioned this in D3582: [services] Backup/Blob - fix handling reading done in server reactors.Apr 13 2022, 3:38 AM
• karol added a child revision: D3719: [services] Backup - client reactors - distinguish terminated from done.Apr 13 2022, 7:12 AM
• karol added a comment.Apr 19 2022, 11:53 PM
In D3664#102317, @palys-swm wrote:

Are we sure that this is properly synchronized and we don't need e.g. atomic?

Good point, we can use it.

• karol updated this revision to Diff 11654.Apr 19 2022, 11:58 PM

add atomic

Harbormaster completed remote builds in B8344: Diff 11654.Apr 20 2022, 12:02 AM
Closed by commit rCOMM61665dd80d82: [services] Backup/Blob - Make sure Finish is only called once (authored by • karol). · Explain WhyApr 21 2022, 1:49 AM
This revision was automatically updated to reflect the committed changes.
• karol added a commit: rCOMM61665dd80d82: [services] Backup/Blob - Make sure Finish is only called once.

Revision Contents
Changeset List

PathSize
services/
backup/
docker-server/
contents/
server/
src/
Reactors/
server/
base-reactors/
5 lines
5 lines
5 lines
blob/
src/
Reactors/
server/
base-reactors/
5 lines
5 lines
5 lines

Diff 11336

View Options

services/backup/docker-server/contents/server/src/Reactors/server/base-reactors/ServerBidiReactorBase.h

Loading...
View Options

services/backup/docker-server/contents/server/src/Reactors/server/base-reactors/ServerReadReactorBase.h

Loading...
View Options

services/backup/docker-server/contents/server/src/Reactors/server/base-reactors/ServerWriteReactorBase.h

Loading...
View Options

services/blob/src/Reactors/server/base-reactors/ServerBidiReactorBase.h

Loading...
View Options

services/blob/src/Reactors/server/base-reactors/ServerReadReactorBase.h

Loading...
View Options

services/blob/src/Reactors/server/base-reactors/ServerWriteReactorBase.h

Loading...