[services] Backup - synchronize status access in reactors
AbandonedPublic
Actions

Authored by • karol on Apr 20 2022, 11:57 PM.

Details

Reviewers

tomek

Summary

Depends on D3789

The issue was raised in https://phabricator.ashoat.com/D3720#103334 and I decided it would be the best to handle synchronization in the mutual code, therefore I first wanted to implement pulling services' mutual code into a single master class.

This is for the backup service.

Test Plan

cd services
yarn run-backup-service

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• karol created this revision.Apr 20 2022, 11:57 PM

Herald added subscribers: • abosh, • benschac, atul and 3 others. · View Herald TranscriptApr 20 2022, 11:57 PM

• karol edited the summary of this revision. (Show Details)Apr 21 2022, 12:01 AM

• karol edited the test plan for this revision. (Show Details)

• karol added a reviewer: tomek.

• karol mentioned this in D3801: [services] Blob - synchronize status access in reactors.

• karol edited the summary of this revision. (Show Details)

• karol added a parent revision: D3789: [services] Blob - Pull more mutual code from base reactors - Apply in server reactors.

• karol added a child revision: D3801: [services] Blob - synchronize status access in reactors.

Harbormaster completed remote builds in B8381: Diff 11693.Apr 21 2022, 12:02 AM

• karol requested review of this revision.Apr 21 2022, 12:02 AM

• karol mentioned this in D3720: [services] Backup - Blob client reactors - add getStatus.Apr 21 2022, 12:03 AM

• karol planned changes to this revision.Apr 21 2022, 3:09 AM

rebase

Harbormaster completed remote builds in B8397: Diff 11713.Apr 21 2022, 3:19 AM

• karol planned changes to this revision.Apr 22 2022, 5:18 AM

rebase

Harbormaster completed remote builds in B8452: Diff 11791.Apr 22 2022, 5:23 AM

rebase incoming

rebase

Harbormaster completed remote builds in B8459: Diff 11798.Apr 22 2022, 5:54 AM

tomek requested changes to this revision.Apr 26 2022, 10:02 AM

tomek added inline comments.

services/backup/src/Reactors/client/base-reactors/ClientBidiReactorBase.h
81–101 ↗	(On Diff #11798)	It seems like we should hold a mutex for the whole duration of this function. I think it will be hard to prove that nothing could go wrong when multiple threads would concurrently set and get the status in this function. For example, one thread can enter the first `if`, then another thread would enter the same `if`, then the first thread would set a status and then the second thread would set it to a different value.