[services] Backup - Fix terminate callback in pull backup reactor
ClosedPublic
Actions

Authored by • karol on Jun 7 2022, 12:26 AM.

Details

Reviewers

tomek

Commits

rCOMMccd926274f68: [services] Backup - Fix terminate callback in pull backup reactor

Summary

We have to check whether the get reactor is initialized in the pull backup reactor before checking its status. The initialization of this reactor happens in the writeResponse method so if we have an error in the initialize method, terminate's going to be called with get reactor uninitialized and that will lead to a crash.

Test Plan

make a request for pulling a backup without passing a backup id (missing any fields would trigger this I think).

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• karol created this revision.Jun 7 2022, 12:26 AM

Herald added subscribers: • abosh, atul, • adrian and 2 others. · View Herald TranscriptJun 7 2022, 12:26 AM

• karol edited the summary of this revision. (Show Details)Jun 7 2022, 12:31 AM

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

• karol added a reviewer: tomek.

CI looks dead at the moment and I'm pretty sure it's going to compile.

In D4223#119338, @karol-bisztyga wrote:

CI looks dead at the moment and I'm pretty sure it's going to compile.

Sorry, the issue here was with the switch from phabricator.ashoat.com => phab.comm.dev. Had to update the webhook endpoint in Buildkite for the build status to get picked up by Phabricator.

Kicked off the builds again... hopefully everything works as expected. Will take a look to see if any other diffs are stuck in this state and kick them off again.

Harbormaster completed remote builds in B9647: Diff 13371.Jun 8 2022, 1:26 AM

tomek accepted this revision.Jun 13 2022, 3:20 AM

tomek added inline comments.

services/backup/src/Reactors/server/PullBackupReactor.cpp
140–141 ↗	(On Diff #13371)	From the summary it looks like `this->getReactor == nullptr` means that there was an error - shouldn't we throw in that case?

This revision is now accepted and ready to land.Jun 13 2022, 3:20 AM

• karol added inline comments.Jul 19 2022, 1:36 AM

services/backup/src/Reactors/server/PullBackupReactor.cpp
140–141 ↗	(On Diff #13371)	No. You're introducing a crash here. If get reactor is null, then reading anything from it in the next line causes a crash. I think it suffices to look at the status in terms of deciding whether an error occurred or not.