grahamc/hydra - Change DRC26KFBZIWIFT5CMAVMCHLDYWHUMKC33R3TW2KEZ5NTSVT2AFOAC

hydra-queue-runner: Fix a race keeping cancelled steps alive

If a step is cancelled just as its builder step is starting, doBuildStep() will return sRetry. This causes builder() to make the step runnable again, since the queue monitor may have added new builds referencing it. The idea is that if the latter condition is not true, the step's reference count will drop to zero and it will be deleted. However, if the dispatcher thread sees and locks the step before the reference count can drop to zero in the builder thread, the dispatcher thread will start a new builder thread for the step. Thus the step can be kept alive for an indefinite amount of time.

The fix is for State::builder() to use a weak pointer to the step, to ensure that the step's reference count can drop to zero before it's added to the runnable queue.

Created by Eelco Dolstra on November 8, 2016

DRC26KFBZIWIFT5CMAVMCHLDYWHUMKC33R3TW2KEZ5NTSVT2AFOAC

Dependencies

In channels

main

upstream

Change contents

Replacement in src/hydra-queue-runner/builder.cc at line 16 [5.133]

B:BD[3.1] → [4.0:96]

B:BD[4.139] → [4.139:184]

    auto activeStep = std::make_shared<ActiveStep>();
    activeStep->step = reservation->step;
    activeSteps_.lock()->insert(activeStep);

[3.1]

[5.350]

    Step::wptr wstep = reservation->step;

Replacement in src/hydra-queue-runner/builder.cc at line 18 [5.133]

B:BD[3.93] → [3.93:130]

B:BD[4.220] → [4.220:268]

∅:D[4.268] → [3.215:223]

B:BD[3.215] → [3.215:223]

    Finally removeActiveStep([&]() {
        activeSteps_.lock()->erase(activeStep);
    });

[5.351]

[6.58]

    {
        auto activeStep = std::make_shared<ActiveStep>();
        activeStep->step = reservation->step;
        activeSteps_.lock()->insert(activeStep);

Replacement in src/hydra-queue-runner/builder.cc at line 23 [5.133]

B:BD[6.59] → [6.59:94]

    auto step = reservation->step;

[6.59]

[5.388]

        Finally removeActiveStep([&]() {
            activeSteps_.lock()->erase(activeStep);
        });

Replacement in src/hydra-queue-runner/builder.cc at line 27 [5.133]

B:BD[5.389] → [5.389:399]

B:BD[5.399] → [7.1152:1193]

B:BD[7.1193] → [2.767:830]

∅:D[8.96] → [5.499:627]

∅:D[6.159] → [5.499:627]

∅:D[2.830] → [5.499:627]

∅:D[7.1261] → [5.499:627]

B:BD[5.499] → [5.499:627]

B:BD[5.627] → [6.160:233]

    try {
        auto destStore = getDestStore();
        res = doBuildStep(destStore, reservation, activeStep);
    } catch (std::exception & e) {
        printMsg(lvlError, format("uncaught exception building ‘%1%’ on ‘%2%’: %3%")
            % step->drvPath % reservation->machine->sshName % e.what());

[5.389]

[5.687]

        try {
            auto destStore = getDestStore();
            res = doBuildStep(destStore, reservation, activeStep);
        } catch (std::exception & e) {
            printMsg(lvlError, format("uncaught exception building ‘%1%’ on ‘%2%’: %3%")
                % reservation->step->drvPath % reservation->machine->sshName % e.what());
        }

Replacement in src/hydra-queue-runner/builder.cc at line 43 [5.133]
B:BD[5.940] → [8.97:121]
```
    if (res != sDone) {
```
[5.940]
[8.121]
```
    Step::ptr step = wstep.lock();
    if (res != sDone && step) {
```