grahamc/hydra2 - Change 2DNPZFPNI2OM5FKYTC2KE5NKKKAP45AQ2VDDYLZZHCJ35X3EBJRQC

Step cancellation: Don't use pthread_cancel()

This was a bad idea because pthread_cancel() is unsalvageable broken in C++. Destructors are not allowed to throw exceptions (especially in C++11), but pthread_cancel() can cause a __cxxabiv1::__forced_unwind exception inside any destructor that invokes a cancellation point. (This exception can be caught but must be rethrown.) So let's just kill the builder process instead.

Created by Eelco Dolstra on November 7, 2016

2DNPZFPNI2OM5FKYTC2KE5NKKKAP45AQ2VDDYLZZHCJ35X3EBJRQC

Dependencies

In channels

main

Change contents

Replacement in src/hydra-queue-runner/build-remote.cc at line 120 [7.85]
B:BD[6.274] → [6.274:301]
```
    RemoteResult & result)
```
[6.274]
[7.2925]
```
    RemoteResult & result, std::shared_ptr<ActiveStep> activeStep)
```

Insertion in src/hydra-queue-runner/build-remote.cc at line 140 [7.85]

[3.553]

[8.23]


        {
            auto activeStepState(activeStep->state_.lock());
            if (activeStepState->cancelled) throw Error("step cancelled");
            activeStepState->pid = child.pid;
        }
        Finally clearPid([&]() {
            auto activeStepState(activeStep->state_.lock());
            activeStepState->pid = -1;
            /* FIXME: there is a slight race here with step
               cancellation in State::processQueueChange(), which
               could call kill() on this pid after we've done waitpid()
               on it. With pid wrap-around, there is a tiny
               possibility that we end up killing another
               process. Meh. */
        });

Deletion in src/hydra-queue-runner/builder.cc at line 18 [9.133]
B:BD[4.96] → [4.96:139]
```
    activeStep->threadId = pthread_self();
```
Deletion in src/hydra-queue-runner/builder.cc at line 21 [9.133]
B:BD[10.130] → [4.185:220]
```
        activeStep->threadId = -1;
```

Replacement in src/hydra-queue-runner/builder.cc at line 28 [9.133]

B:BD[11.1193] → [2.30:96]

        res = doBuildStep(destStore, step, reservation->machine);

[11.1193]

[9.499]

        res = doBuildStep(destStore, reservation, activeStep);

Replacement in src/hydra-queue-runner/builder.cc at line 57 [9.133]

B:BD[9.1479] → [2.151:231]

∅:D[2.231] → [9.1552:1578]

∅:D[12.694] → [9.1552:1578]

∅:D[11.1329] → [9.1552:1578]

B:BD[9.1552] → [9.1552:1578]

State::StepResult State::doBuildStep(nix::ref<Store> destStore, Step::ptr step,
    Machine::ptr machine)

[9.1479]

[9.1578]

State::StepResult State::doBuildStep(nix::ref<Store> destStore,
    MachineReservation::ptr reservation,
    std::shared_ptr<ActiveStep> activeStep)

Insertion in src/hydra-queue-runner/builder.cc at line 61 [9.133]

[9.1580]

    auto & step(reservation->step);
    auto & machine(reservation->machine);

Replacement in src/hydra-queue-runner/builder.cc at line 161 [9.133]

B:BD[9.4376] → [10.1142:1230]

            buildRemote(destStore, machine, step, maxSilentTime, buildTimeout, result);

[9.4376]

[13.2415]

            buildRemote(destStore, machine, step, maxSilentTime, buildTimeout, result, activeStep);

Replacement in src/hydra-queue-runner/builder.cc at line 165 [9.133]

B:BD[9.4504] → [13.2505:2548]

∅:D[13.2548] → [9.4558:4597]

B:BD[9.4558] → [9.4558:4597]

B:BD[9.4597] → [14.0:36]

B:BD[14.36] → [10.1231:1365]

            result.stepStatus = bsAborted;
            result.errorMsg = e.msg();
            result.canRetry = true;
        } catch (__cxxabiv1::__forced_unwind & e) {
            /* The queue monitor thread cancelled this step. */
            try {

[9.4504]

[5.0]

            if (activeStep->state_.lock()->cancelled) {

Replacement in src/hydra-queue-runner/builder.cc at line 167 [9.133]

∅:D[5.88] → [10.1453:1776]

B:BD[10.1453] → [10.1453:1776]

                pqxx::work txn(*conn);
                finishBuildStep(txn, result.startTime, time(0), result.overhead, buildId,
                    stepNr, machine->sshName, bsCancelled, "");
                txn.commit();
                stepFinished = true;
            } catch (...) {
                ignoreException();

[5.88]

[10.1776]

                result.stepStatus = bsCancelled;
                result.canRetry = false;
            } else {
                result.stepStatus = bsAborted;
                result.errorMsg = e.msg();
                result.canRetry = true;

Deletion in src/hydra-queue-runner/builder.cc at line 174 [9.133]
B:BD[10.1790] → [10.1790:1809]
```
            throw;
```

Deletion in src/hydra-queue-runner/queue-monitor.cc at line 340 [9.20067]

B:BD[10.4131] → [10.4131:4253]

            auto threadId = activeStep->threadId; // FIXME: use Sync or atomic?
            if (threadId == 0) continue;

Replacement in src/hydra-queue-runner/queue-monitor.cc at line 345 [9.20067]

B:BD[10.4449] → [10.4449:4759]

            printInfo("cancelling thread for build step ‘%s’", activeStep->step->drvPath);
            int err = pthread_cancel(threadId);
            if (err)
                printError("error cancelling thread for build step ‘%s’: %s",
                    activeStep->step->drvPath, strerror(err));

[10.4449]

[15.1940]

            {
                auto activeStepState(activeStep->state_.lock());
                if (activeStepState->cancelled) continue;
                activeStepState->cancelled = true;
                if (activeStepState->pid != -1) {
                    printInfo("killing builder process %d of build step ‘%s’",
                        activeStepState->pid, activeStep->step->drvPath);
                    if (kill(activeStepState->pid, SIGINT) == -1)
                        printError("error killing build step ‘%s’: %s",
                            activeStep->step->drvPath, strerror(errno));
                }
            }

Replacement in src/hydra-queue-runner/state.hh at line 380 [16.1128]

B:BD[4.321] → [4.321:349]

        pthread_t threadId;

[4.321]

[4.349]


        struct State
        {
            pid_t pid = -1;
            bool cancelled = false;
        };
        nix::Sync<State> state_;

Replacement in src/hydra-queue-runner/state.hh at line 486 [16.1128]

B:BD[2.431] → [2.431:478]

        Step::ptr step, Machine::ptr machine);

[2.431]

[16.8478]

        MachineReservation::ptr reservation,
        std::shared_ptr<ActiveStep> activeStep);

Replacement in src/hydra-queue-runner/state.hh at line 492 [16.1128]

B:BD[6.1984] → [6.1984:2016]

        RemoteResult & result);

[6.1984]

[6.2016]

        RemoteResult & result, std::shared_ptr<ActiveStep> activeStep);