Details

Reviewers

• max
varun
tomek
• jim

Commits

rCOMM0cc54d3232c5: [services] Backup - Fix premature errors crash

Summary

Depends on D3532

In CreateNewBackupReactor when an error occurs before the putReactor is created, there is a crash, because we try to use the putReactor in doneCallback but it's nullptr.

Test Plan

cd services
yarn run-backup-service

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• karol created this revision.Mar 29 2022, 4:39 AM

Herald added subscribers: • abosh, • benschac, atul and 3 others. · View Herald TranscriptMar 29 2022, 4:39 AM

• karol edited the summary of this revision. (Show Details)Mar 29 2022, 4:51 AM

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

• karol added reviewers: • max, varun, tomek, • jim.

• karol added a parent revision: D3532: [services] Backup - Move Service Blob Client.

• karol added a child revision: D3534: [services] Backup - Print out errors in reactors.

Harbormaster completed remote builds in B7670: Diff 10766.Mar 29 2022, 4:55 AM

• karol requested review of this revision.Mar 29 2022, 4:55 AM

If we can't execute doneCallback maybe we should not call it at all?

This revision now requires changes to proceed.Apr 1 2022, 4:22 AM

rebase

still need to address the feedback

Harbormaster failed remote builds in B7817: Diff 10984!Apr 4 2022, 3:25 AM

In D3533#98253, @palys-swm wrote:

If we can't execute doneCallback maybe we should not call it at all?

What do you mean?

• karol requested review of this revision.Apr 4 2022, 11:30 PM

In D3533#98617, @karol-bisztyga wrote:

In D3533#98253, @palys-swm wrote:

If we can't execute doneCallback maybe we should not call it at all?

What do you mean?

In the summary you mentioned that

In CreateNewBackupReactor when an error occurs before the putReactor is created, there is a crash, because we try to use the putReactor in doneCallback but it's nullptr.

Which means that the code crashes because we call doneCallback in CreateNewBackupReactor after an error. I was asking if it makes sense to completely avoid calling doneCallback in that case instead of adding early return to it. Do you think it makes sense? Which solution do you prefer?

This revision now requires changes to proceed.Apr 5 2022, 9:26 AM

This is against the whole idea behind the doneCallback - it should be called regardless of whether an error has been thrown or not. In this callback, we can access this->status and see if there was an error or not.
The purpose of this callback is to perform operations when a reactor is finishing and there are no more grpc operations left.

In D3533#99379, @karol-bisztyga wrote:

This is against the whole idea behind the doneCallback - it should be called regardless of whether an error has been thrown or not. In this callback, we can access this->status and see if there was an error or not.
The purpose of this callback is to perform operations when a reactor is finishing and there are no more grpc operations left.

Ok, that makes sense! Thanks for explaining it