Paths

Table of Contentst

Differential D3664

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

Authored by • karol on Apr 8 2022, 9:33 AM.

Tags

None

Referenced Files

	F32238275: D3664.1765224017.diff
	Sun, Dec 7, 8:00 PM

Subscribers

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• karol created this revision.Apr 8 2022, 9:33 AM

Herald added subscribers: • abosh, • benschac, atul and 3 others. · View Herald TranscriptApr 8 2022, 9:33 AM

• karol edited the summary of this revision. (Show Details)Apr 8 2022, 9: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, 9:48 AM

• karol requested review of this revision.Apr 8 2022, 9:48 AM

ashoat added inline comments.Apr 11 2022, 2:01 AM

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`

(I only mentioned four locations but there are more)

This revision now requires changes to proceed.Apr 11 2022, 2:01 AM

add this->

Harbormaster completed remote builds in B8109: Diff 11336.Apr 12 2022, 8:35 AM

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

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, 9:48 AM

• karol mentioned this in D3582: [services] Backup/Blob - fix handling reading done in server reactors.Apr 13 2022, 10:38 AM

• karol added a child revision: D3719: [services] Backup - client reactors - distinguish terminated from done.Apr 13 2022, 2:12 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.

add atomic

Harbormaster completed remote builds in B8344: Diff 11654.Apr 20 2022, 7:02 AM

Closed by commit rCOMM61665dd80d82: [services] Backup/Blob - Make sure Finish is only called once. · Explain WhyApr 21 2022, 8: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

Path

Size

services/

backup/

docker-server/

contents/

server/

src/

Reactors/

server/

base-reactors/

ServerBidiReactorBase.h

6 lines

ServerReadReactorBase.h

6 lines

ServerWriteReactorBase.h

6 lines

blob/

src/

Reactors/

server/

base-reactors/

ServerBidiReactorBase.h

6 lines

ServerReadReactorBase.h

6 lines

ServerWriteReactorBase.h

6 lines

Diff 11704

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

Loading...

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

Loading...

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

Loading...

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

Loading...

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

Loading...

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

Loading...