Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Graceful shutdown on interrupt #46

Open
enumag opened this issue Apr 25, 2018 · 14 comments
Open

Graceful shutdown on interrupt #46

enumag opened this issue Apr 25, 2018 · 14 comments

Comments

@enumag
Copy link
Member

enumag commented Apr 25, 2018

I just encountered an issue where one of my projection was in an invalid state after being interrupted during previous run. The state of the projection was not saved to the database correctly

I believe the projection commands in this bundle could and should catch interruption signals like Ctrl+C and shutdown correctly when it happens using pcntl_signal function. Something like this (by @prolic):

pcntl_signal(SIGINT, function () use ($projection) {
    $projection->stop();
});

I'm not using the projection commands from this bundle at the moment but I'm going to refactor my project to use them soon. I might take a look into this then too.

Any tips what I should be aware of? I never tried working with pcntl_signal function until now.

@UFOMelkor
Copy link
Member

Unfortunately I've never tried workwing with it too.
But this is definitively something we should look for.

@prolic
Copy link
Member

prolic commented Apr 25, 2018

@UFOMelkor it's really as simple as:

pcntl_signal(SIGINT, function () use ($projection) {
    $projection->stop();
});

$projection->run($keepRunning);

@prolic
Copy link
Member

prolic commented Apr 25, 2018

Note:

Ctrl+C - SIGINT
Ctrl+\ - SIGQUIT

@enumag
Copy link
Member Author

enumag commented Apr 26, 2018

I looked into this for a little bit. It only works when $options[PdoEventStoreReadModelProjector::OPTION_PCNTL_DISPATCH] is set to true. Currently it's impossible to set it that way because AbstractProjectionCommand doesn't pass any options when calling ProjectionManager::createReadModelProjection().

@enumag
Copy link
Member Author

enumag commented Apr 26, 2018

Another thing is that pcntl_signal_dispatch(); doesn't seem to be called often enough. I believe it should be added to the end of ReadModelProjector::persist(). Can you think of any possible problems with that?

@enumag
Copy link
Member Author

enumag commented May 2, 2018

ping @prolic @UFOMelkor

@fritz-gerneth
Copy link

fritz-gerneth commented May 2, 2018

If you want to handle all scenarios for interrupts and means of termination you will have to add additional signals to listen for (SIGTERM, SIGINT, SIGQUIT, (SIGHUB)). Only listening to SIGINT won't catch all instances of how a process can be forced to quit.
The places where the current dipatches are located are quite intentional: before and after one full loop of processing. Having this in between might cause inconsitent read models in some scenarios (depending on how they are implemented).

This is a run wrapper we use on our end:

class ProjectionRunWrapper
{
    /** @var  string */
    private $projectionName;

    /** @var  Query|Projector|ReadModelProjector */
    private $projection;

    /** @var  Logger */
    private $logger;

    public function __construct(string $projectionName, $projection, Logger $logger)
    {
        $this->projectionName = $projectionName;
        $this->projection = $projection;
        $this->logger = $logger;
    }

    public function run(): void
    {
        $this->logger->info(
            sprintf('Starting projection \'%s\'', $this->projectionName),
            ['projection' => $this->projectionName, ]
        );

        $this->registerSignalHandlers();

        try {
            $this->projection->run();
        } catch (\Throwable $e) {
            $this->logger->err(
                $e->getMessage(),
                ['projection' => $this->projectionName, 'exception' => $e, ]
            );
            $this->finalize();
        }
    }

    private function registerSignalHandlers(): void
    {
        $pcntlCallback = function () { $this->handleSignals(); };

        pcntl_signal(SIGHUP, $pcntlCallback);
        pcntl_signal(SIGTERM, $pcntlCallback);
        pcntl_signal(SIGINT, $pcntlCallback);
        pcntl_signal(SIGQUIT, $pcntlCallback);
    }

    private function handleSignals(): void
    {
        $this->logger->info(
            sprintf('Attempting graceful shutdown of projection \'%s\'', $this->projectionName),
            ['projection' => $this->projectionName, ]
        );
        if (null === $this->projection) {
            $this->logger->err(
                sprintf('Projection \'%s\' not started', $this->projectionName).
                ['projection' => $this->projectionName, ]
            );
            return;
        }
        $this->finalize();
    }

    private function finalize(): void
    {
        $this->logger->info(
            sprintf( 'Projection \'%s\' stopped', $this->projectionName),
            ['projection' => $this->projectionName, ]
        );
        $this->projection->stop();
        $this->projection = null;
    }
}

@enumag
Copy link
Member Author

enumag commented May 3, 2018

If you want to handle all scenarios for interrupts and means of termination

That's not really the goal here. I just want a fix for regular interrupt.

The places where the current dipatches are located are quite intentional: before and after one full loop of processing. Having this in between might cause inconsitent read models in some scenarios (depending on how they are implemented).

What scenario would be broken by dispatch in the projector persist method? As far as I can tell it seems to be a safe place for it.

This is a run wrapper we use on our end:

I was thinking about something like that too but I still prefer to have it fixed in prooph. Besides this doesn't fix the problem of too long time between dispatch calls which is the main issue.

@fritz-gerneth
Copy link

fritz-gerneth commented May 3, 2018

What scenario would be broken by dispatch in the projector persist method? As far as I can tell it seems to be a safe place for it.

That's an interrupt between applying the data and saving the information this data has been applied. As this is not running within an transaction, quiting before saving the state would result in an event applied in the read model (e.g. row created) but the state not persisted. E.g. on next start the event would be re-applied again. Now this is all old "at least once" delivery and "make your read models idempotent and can be handled to some extend in SQL directly.

I'm wondering about how did your read model get corrupted? pcntl_signal_dispatch is indeed only invoked after all events from all streams have been processed, and before new ones checked. The only scenario I could think of is that you had to process quite a few events in one batch and some process manager killed the projection because it did not respond to SIGINT in time?

@prolic
Copy link
Member

prolic commented May 3, 2018

@fritz-gerneth that's no longer true, see prooph/pdo-event-store#144

@prolic
Copy link
Member

prolic commented May 3, 2018

@fritz-gerneth the change is released with v1.8.1 (https://github.com/prooph/pdo-event-store/releases/tag/v1.8.1)

@fritz-gerneth
Copy link

And here I was sitting on 1.8.0 :) v1.8.1 very well might solve this issue then :)

@enumag
Copy link
Member Author

enumag commented May 3, 2018

I missed that PR too. That should solve my issues with dispatch then.

Next we need a way to configure the options with this bundle:

Currently it's impossible to set it that way because AbstractProjectionCommand doesn't pass any options when calling ProjectionManager::createReadModelProjection().

@adapik
Copy link

adapik commented May 15, 2018

I think we should pass options to createReadModelProjection and define them in bundle config/parameters (event_store.xml (?)). Also we should be able to register custom signal handlers (collect them via tag). I suppose signal handlers should be placed in your application but we can place SIGINT handler into the symfony-event-store-bundle by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants