Optionally trigger pcntl_signal_dispatch #96

fritz-gerneth · 2017-06-29T08:40:42Z

Currently there is no way to gracefully shut down a running projection when any OS signals are received. Setting the projection is stopping state via a custom CLI script is not always an option (e.g. when projections and others are managed by supervisord).

I think how the different signals should be handled should be left to the programmer, I have included a short example for reference below. In any case, to support this feature the projection loop at least needs to trigger the signal dispatch function.

Some remarks on my approach:

declare(ticks=1) is not required anymore to my knowledge, it got replaced by pcntl_signal_dispatch
I made this argument optional, defaulting to false to remain fully backwards compatible
The location of the triggerPcntlSignalDispatch call within the loop has quite a few implications. It might even make sense to trigger this before usleep.

        $pcntlCallback = function () use ($name) {
            echo "Attempting graceful shutdown of projection " . $name . PHP_EOL;
            $this->currentProjection->stop();
        };
        pcntl_signal(SIGTERM, $pcntlCallback);
        pcntl_signal(SIGINT, $pcntlCallback);
        pcntl_signal(SIGQUIT, $pcntlCallback);

      $this->currentProjection->start();

coveralls · 2017-06-29T08:49:09Z

Coverage decreased (-0.07%) to 92.497% when pulling b0c4eef on funct:pcntl_signals into 1f9702d on prooph:master.

codeliner · 2017-06-29T11:02:38Z

src/Projection/PdoEventStoreProjector.php

@@ -164,6 +170,7 @@ public function __construct(
        $this->persistBlockSize = $persistBlockSize;
        $this->sleep = $sleep;
        $this->status = ProjectionStatus::IDLE();
+        $this->triggerPcntlSignalDispatch = extension_loaded('pcntl') && $triggerPcntlSignalDispatch;


switch the order of the checks so that if $triggerPcntlSignalDispatch = false PHP does not check if extension is loaded

codeliner · 2017-06-29T11:02:59Z

src/Projection/PdoEventStoreReadModelProjector.php

@@ -151,6 +161,7 @@ public function __construct(
        $this->persistBlockSize = $persistBlockSize;
        $this->sleep = $sleep;
        $this->status = ProjectionStatus::IDLE();
+        $this->triggerPcntlSignalDispatch = extension_loaded('pcntl') && $triggerPcntlSignalDispatch;


same as above

coveralls · 2017-06-29T11:20:12Z

Coverage decreased (-0.8%) to 91.743% when pulling 33b410b on funct:pcntl_signals into 1f9702d on prooph:master.

codeliner · 2017-06-29T11:41:36Z

@prolic can you take a look? You are the expert in that area.

Also a test and docs update would be nice, especially regarding $pcntlCallback

prolic

besides some small remarks, it definitely needs docs and a small sample in the docs on how usage might look like

prolic · 2017-06-29T15:28:44Z

src/Projection/PdoEventStoreReadModelProjector.php

@@ -35,6 +35,10 @@

 final class PdoEventStoreReadModelProjector implements ReadModelProjector
 {
+    const OPTION_PCNTL_DISPATCH = 'trigger_pcntl_dispatch';


public const

prolic · 2017-06-29T15:28:49Z

src/Projection/PdoEventStoreReadModelProjector.php

@@ -35,6 +35,10 @@

 final class PdoEventStoreReadModelProjector implements ReadModelProjector
 {
+    const OPTION_PCNTL_DISPATCH = 'trigger_pcntl_dispatch';
+
+    const DEFAULT_PCNTL_DISPATCH = false;


public const

prolic · 2017-06-29T15:29:58Z

src/Projection/PdoEventStoreProjector.php

+     */
+    private $triggerPcntlSignalDispatch;
+
+    /**


new consts missing

Not sure I follow, could you ellaborate? :)

prolic · 2017-06-29T15:30:19Z

src/Projection/MySqlProjectionManager.php

@@ -87,7 +87,8 @@ public function createProjection(
            $options[PdoEventStoreProjector::OPTION_LOCK_TIMEOUT_MS] ?? PdoEventStoreProjector::DEFAULT_LOCK_TIMEOUT_MS,
            $options[PdoEventStoreProjector::OPTION_CACHE_SIZE] ?? PdoEventStoreProjector::DEFAULT_CACHE_SIZE,
            $options[PdoEventStoreProjector::OPTION_PERSIST_BLOCK_SIZE] ?? PdoEventStoreProjector::DEFAULT_PERSIST_BLOCK_SIZE,
-            $options[PdoEventStoreProjector::OPTION_SLEEP] ?? PdoEventStoreProjector::DEFAULT_SLEEP
+            $options[PdoEventStoreProjector::OPTION_SLEEP] ?? PdoEventStoreProjector::DEFAULT_SLEEP,
+            $options[PdoEventStoreReadModelProjector::OPTION_PCNTL_DISPATCH] ?? PdoEventStoreReadModelProjector::DEFAULT_PCNTL_DISPATCH


wrong cost usage

Not sure I follow, could you ellaborate? :)

prolic · 2017-06-29T15:30:30Z

src/Projection/MariaDbProjectionManager.php

@@ -87,7 +87,8 @@ public function createProjection(
            $options[PdoEventStoreProjector::OPTION_LOCK_TIMEOUT_MS] ?? PdoEventStoreProjector::DEFAULT_LOCK_TIMEOUT_MS,
            $options[PdoEventStoreProjector::OPTION_CACHE_SIZE] ?? PdoEventStoreProjector::DEFAULT_CACHE_SIZE,
            $options[PdoEventStoreProjector::OPTION_PERSIST_BLOCK_SIZE] ?? PdoEventStoreProjector::DEFAULT_PERSIST_BLOCK_SIZE,
-            $options[PdoEventStoreProjector::OPTION_SLEEP] ?? PdoEventStoreProjector::DEFAULT_SLEEP
+            $options[PdoEventStoreProjector::OPTION_SLEEP] ?? PdoEventStoreProjector::DEFAULT_SLEEP,
+            $options[PdoEventStoreReadModelProjector::OPTION_PCNTL_DISPATCH] ?? PdoEventStoreReadModelProjector::DEFAULT_PCNTL_DISPATCH


wrong const usage

prolic · 2017-06-29T15:30:55Z

src/Projection/PostgresProjectionManager.php

@@ -87,7 +87,8 @@ public function createProjection(
            $options[PdoEventStoreProjector::DEFAULT_LOCK_TIMEOUT_MS] ?? PdoEventStoreProjector::DEFAULT_LOCK_TIMEOUT_MS,
            $options[PdoEventStoreProjector::OPTION_CACHE_SIZE] ?? PdoEventStoreProjector::DEFAULT_CACHE_SIZE,
            $options[PdoEventStoreProjector::OPTION_PERSIST_BLOCK_SIZE] ?? PdoEventStoreProjector::DEFAULT_PERSIST_BLOCK_SIZE,
-            $options[PdoEventStoreProjector::OPTION_SLEEP] ?? PdoEventStoreProjector::DEFAULT_SLEEP
+            $options[PdoEventStoreProjector::OPTION_SLEEP] ?? PdoEventStoreProjector::DEFAULT_SLEEP,
+            $options[PdoEventStoreReadModelProjector::OPTION_PCNTL_DISPATCH] ?? PdoEventStoreReadModelProjector::DEFAULT_PCNTL_DISPATCH


wrong const usage

prolic · 2017-06-29T15:34:36Z

src/Projection/PdoEventStoreProjector.php

@@ -498,6 +505,8 @@ public function run(bool $keepRunning = true): void

                $this->eventCounter = 0;

+                $this->triggerPcntlSignalDispatch();


I think this might be the best place for that function call

prolic · 2017-06-29T15:35:05Z

@fritz-gerneth do you have any idea how to add a test for this ?

prolic · 2017-06-29T15:52:13Z

Should be another const in PdoEventStoreProjector, or even at interface level.

…

On Jun 29, 2017 11:49 PM, "Fritz Gerneth" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Projection/PdoEventStoreProjector.php <#96 (comment)>: > @@ -134,6 +134,11 @@ private $sleep; /** + * @var bool + */ + private $triggerPcntlSignalDispatch; + + /** Not sure I follow, could you ellaborate? :) — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#96 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAYEvJaokUyioOndD8mOSZdkxGJrAechks5sI8ehgaJpZM4OI_dg> .

fritz-gerneth · 2017-06-29T15:58:07Z

Certainly can add some documentation. Any prefered location or just create a new file "Using from CLI"?

For testing - unfortunately I don't have any idea that does not require dirty hacks. The two problems I see:

How to assert invocation of a core function (without rootkit, namespacing tricks) when doing a UT
How spawn a new process running the projection from a test and send send a kill signal as an integration test. Technically we sure can spawn a new child process and kill it. But pcntl functions are known to cause a bit of a headache when child processes install signal handlers (e.g. this comment)
How to test for extension loaded / not loaded

I sure could change this to remove the dependency on pcntl_signal_dispatch and replace it with some generic lifecycle callback that can be passed to the constructor instead. I find this quite obfuscated though :)

coveralls · 2017-06-29T16:22:32Z

Coverage decreased (-0.8%) to 91.743% when pulling 1c98bde on funct:pcntl_signals into 1f9702d on prooph:master.

prolic · 2017-06-29T17:33:18Z

src/Projection/PdoEventStoreProjector.php

@@ -38,6 +38,10 @@

 final class PdoEventStoreProjector implements Projector
 {
+    public const OPTION_PCNTL_DISPATCH = 'trigger_pcntl_dispatch';


I think the consts should go here: https://github.com/prooph/event-store/blob/master/src/Projection/Projector.php#L25

It's useful for all implementations, not specific to pdo

Thought about that too, they definitely belong there and are helpful for others. This would require a new release of the event-store and update the minimum version required in this package though (else the constants in the projection managers are not defined). If you prefer this I certainly can draft this up.

prolic · 2017-06-29T17:38:17Z

@fritz-gerneth A unit test is not possible, I agree, but we could add an integration test. So spin up an in-memory event-store, run some test projection on it, send a kill signal and see if it stopped gracefully.

hint:

$pid = exec('php test-projector.php &', $output);
posix_kill($pid, SIGUSR1);

prolic · 2017-06-30T06:51:58Z

Go for it!

…

On Jun 30, 2017 04:14, "Fritz Gerneth" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Projection/PdoEventStoreProjector.php <#96 (comment)>: > @@ -38,6 +38,10 @@ final class PdoEventStoreProjector implements Projector { + public const OPTION_PCNTL_DISPATCH = 'trigger_pcntl_dispatch'; Thought about that too, they definitely belong there and are helpful for others. This would require a new release of the event-store and update the minimum version required in this package though (else the constants in the projection managers are not defined). If you prefer this I certainly can draft this up. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#96 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAYEvHWjyO9hg8-d6FLFUmGCegGcXxULks5sJAWNgaJpZM4OI_dg> .

fritz-gerneth · 2017-06-30T11:22:32Z

tests/Projection/PdoEventStoreProjectorTest.php

+         * Created process inherits env variables from this process.
+         * Script returns with non-standard code SIGUSR1 from the handler and -1 else
+         */
+        $projectionProcess = proc_open($command, $descriptorSpec, $pipes);


This turned out to be harder than I thought and brought me down a road I certainly don't want to head to :)

Problem with exec is that it waits for the command to finish and doesn't return the pid. We could resolve this by sending the command to the background (php ... &) or return the pid (php ... & echo $!;). But still then, it doesn't return if the output is not redirected to /dev/null (see comment in documentation). SO I opted for the proc* family instead.

The test itself is rather simple - we return a non-standard exit code in our callback instead of the default one. Disabling the flag on the projection makes this test fail.

prolic

I really like that test-case, just some notes.

prolic · 2017-06-30T13:23:49Z

src/Projection/PdoEventStoreReadModelProjector.php

@@ -151,6 +161,7 @@ public function __construct(
        $this->persistBlockSize = $persistBlockSize;
        $this->sleep = $sleep;
        $this->status = ProjectionStatus::IDLE();
+        $this->triggerPcntlSignalDispatch = $triggerPcntlSignalDispatch && extension_loaded('pcntl');


if $triggerPcntlSignalDispatch is set to true but extension is missing, we should maybe throw an exception instead.

I think this would limit portability of projections across systems (e.g. when a library defines a projection with this flag enabled, while the user's system doesn't have it enabled). Personally I prefer the degraded functionality approach, but it's your call.

see: https://github.com/prooph/event-store/blob/master/src/Projection/InMemoryEventStoreProjector.php#L124-L126

prolic · 2017-06-30T13:25:43Z

tests/Projection/PdoEventStoreReadModelProjectorTest.php

+    /**
+     * @test
+     */
+    public function it_dispatches_pcntl_signals_when_enabled(): void


We should move these tests to the AbstractReadModelProjectorTest in the event-store repo. This way the test can be reused across all event-store implementations. If you think InMemoryEventStore is not worth adding this feature, we simply skip the tests for it.

prolic · 2017-06-30T13:26:28Z

tests/Projection/isolated-read-model-projection.php

+
+require __DIR__ . '/../../vendor/autoload.php';
+
+$readModel = new class() implements \Prooph\EventStore\Projection\ReadModel {


import Prooph\EventStore\Projection\ReadModel

prolic · 2017-06-30T13:43:15Z

@codeliner your thoughts? On Jun 30, 2017 9:41 PM, "Fritz Gerneth" <[email protected]> wrote: *@fritz-gerneth* commented on this pull request. ------------------------------ In src/Projection/PdoEventStoreReadModelProjector.php <#96 (comment)>:

@@ -151,6 +161,7 @@ public function __construct(

$this->persistBlockSize = $persistBlockSize; $this->sleep = $sleep; $this->status = ProjectionStatus::IDLE(); + $this->triggerPcntlSignalDispatch = $triggerPcntlSignalDispatch && extension_loaded('pcntl'); I think this would limit portability of projections across systems (e.g. when a library defines a projection with this flag enabled, while the user's system doesn't have it enabled). Personally I prefer the degraded functionality approach, but it's your call. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#96 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAYEvBAKXhhXyiCF2VFoKa23_AkgkEDtks5sJPsggaJpZM4OI_dg> .

prolic · 2017-07-01T09:39:57Z

Require https://github.com/prooph/event-store/releases/tag/v7.2.0 and update according to changes made there.

…ll as tests based on [email protected] changes

prolic

nearly finished :)

prolic · 2017-07-02T06:39:18Z

src/Projection/PdoEventStoreProjector.php

@@ -164,6 +170,7 @@ public function __construct(
        $this->persistBlockSize = $persistBlockSize;
        $this->sleep = $sleep;
        $this->status = ProjectionStatus::IDLE();
+        $this->triggerPcntlSignalDispatch = $triggerPcntlSignalDispatch && extension_loaded('pcntl');


see: https://github.com/prooph/event-store/blob/master/src/Projection/InMemoryEventStoreProjector.php#L124-L126

prolic · 2017-07-02T06:39:46Z

src/Projection/PdoEventStoreProjector.php

@@ -857,4 +866,11 @@ private function prepareStreamPositions(): void

        $this->streamPositions = array_merge($streamPositions, $this->streamPositions);
    }
+
+    private function triggerPcntlSignalDispatch(): void


Remove this private function and call it directly. It has only a single usage.

prolic · 2017-07-02T06:39:54Z

src/Projection/PdoEventStoreReadModelProjector.php

@@ -151,6 +161,7 @@ public function __construct(
        $this->persistBlockSize = $persistBlockSize;
        $this->sleep = $sleep;
        $this->status = ProjectionStatus::IDLE();
+        $this->triggerPcntlSignalDispatch = $triggerPcntlSignalDispatch && extension_loaded('pcntl');


see: https://github.com/prooph/event-store/blob/master/src/Projection/InMemoryEventStoreProjector.php#L124-L126

prolic · 2017-07-02T06:40:08Z

src/Projection/PdoEventStoreReadModelProjector.php

@@ -807,4 +816,11 @@ private function prepareStreamPositions(): void

        $this->streamPositions = array_merge($streamPositions, $this->streamPositions);
    }
+
+    private function triggerPcntlSignalDispatch(): void


same here. we don't need it as method.

prolic · 2017-07-02T06:41:00Z

tests/Projection/isolated-read-model-projection.php

@@ -0,0 +1,74 @@
+<?php
+


docheader missing

prolic · 2017-07-02T06:41:33Z

tests/Projection/isolated-projection.php

@@ -0,0 +1,45 @@
+<?php
+


docheader missing

… projections

coveralls · 2017-07-02T22:29:26Z

Coverage decreased (-1.0%) to 91.612% when pulling 5432dc9 on funct:pcntl_signals into 1f9702d on prooph:master.

prolic · 2017-07-03T10:31:17Z

thanks a lot! 👍

Optionally trigger pcntl_signal_dispatch

b0c4eef

codeliner requested changes Jun 29, 2017

View reviewed changes

Only check for PHP extension if signal dispatching is enabled

33b410b

codeliner approved these changes Jun 29, 2017

View reviewed changes

codeliner requested a review from prolic June 29, 2017 11:38

prolic requested changes Jun 29, 2017

View reviewed changes

prolic reviewed Jun 29, 2017

View reviewed changes

prolic self-assigned this Jun 29, 2017

prolic added the enhancement label Jun 29, 2017

Replicate config constants to correct interfaces, make them public

1c98bde

prolic reviewed Jun 29, 2017

View reviewed changes

Add tests for pcntl signal dispatching

c40a306

fritz-gerneth commented Jun 30, 2017

View reviewed changes

Skip PCNLT tests if extension is not loaded

56ce637

fritz-gerneth mentioned this pull request Jun 30, 2017

Add projection option constants to trigger pcntl_signal_dispatch prooph/event-store#293

Merged

prolic requested changes Jun 30, 2017

View reviewed changes

fritz-gerneth added 2 commits July 1, 2017 15:16

Update version requirement of prooph/event-store to ^7.2

3f995ad

Update PdoEventStoreProjector & PdoEventStoreReadModelProjector as we…

2514676

…ll as tests based on [email protected] changes

prolic requested changes Jul 2, 2017

View reviewed changes

Minor code-formating & adjustmnets to follow same style than InMemory…

5432dc9

… projections

prolic approved these changes Jul 3, 2017

View reviewed changes

prolic merged commit 6ad9281 into prooph:master Jul 3, 2017

		@@ -498,6 +505,8 @@ public function run(bool $keepRunning = true): void

		$this->eventCounter = 0;

		$this->triggerPcntlSignalDispatch();


		require __DIR__ . '/../../vendor/autoload.php';

		$readModel = new class() implements \Prooph\EventStore\Projection\ReadModel {

Optionally trigger pcntl_signal_dispatch #96

Optionally trigger pcntl_signal_dispatch #96

Conversation

fritz-gerneth commented Jun 29, 2017

coveralls commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 29, 2017

codeliner commented Jun 29, 2017

prolic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prolic commented Jun 29, 2017

prolic commented Jun 29, 2017 via email

fritz-gerneth commented Jun 29, 2017

coveralls commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prolic commented Jun 29, 2017 • edited Loading

prolic commented Jun 30, 2017 via email

Choose a reason for hiding this comment

prolic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prolic commented Jun 30, 2017 via email

prolic commented Jul 1, 2017

prolic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 2, 2017

prolic commented Jul 3, 2017

prolic commented Jun 29, 2017 •

edited

Loading