Fix pcntl_signal() restore when pcntl_alarm() is set

Author: mvorisek

src/Util/PcntlTimeout.php

@@ -53,34 +53,39 @@ public function __construct(int $timeout)
      *
      * @return T
      *
+     * @throws \Throwable
      * @throws DeadlineException    Running the code hit the deadline
      * @throws LockAcquireException Installing the timeout failed
      */
     public function timeBoxed(callable $code)
     {
-        $existingHandler = pcntl_signal_get_handler(\SIGALRM);
+        if (pcntl_alarm($this->timeout) !== 0) {
+            throw new LockAcquireException('Existing process alarm is not supported');
+        }
+
+        $origSignalHandler = pcntl_signal_get_handler(\SIGALRM);
 
-        $signal = pcntl_signal(\SIGALRM, function (): void {
+        $timeout = $this->timeout;
+        $signalHandlerFx = static function () use ($timeout): void {
             throw new DeadlineException(sprintf(
                 'Timebox hit deadline of %d seconds',
-                $this->timeout
+                $timeout
             ));
-        });
-        if (!$signal) {
-            throw new LockAcquireException('Could not install signal');
-        }
+        };
 
-        $oldAlarm = pcntl_alarm($this->timeout);
-        if ($oldAlarm !== 0) {
-            throw new LockAcquireException('Existing alarm was not expected');
+        if (!pcntl_signal(\SIGALRM, $signalHandlerFx)) {
+            throw new LockAcquireException('Failed to install signal handler');
         }
 
         try {
             return $code();
         } finally {
             pcntl_alarm(0);
-            pcntl_signal_dispatch();
-            pcntl_signal(\SIGALRM, $existingHandler);
+            try {
+                pcntl_signal_dispatch();
+            } finally {
+                pcntl_signal(\SIGALRM, $origSignalHandler);
+            }
         }
     }
 

tests/Util/LoopTest.php

@@ -46,7 +46,6 @@ public function testInvalidAcquireTimeout(float $acquireTimeout): void
 
         $this->expectException(\InvalidArgumentException::class);
         $this->expectExceptionMessage('The lock acquire timeout must be greater than or equal to 0.0 (' . LockUtil::getInstance()->formatTimeout($acquireTimeout) . ' was given)');
-
         $loop->execute(static function () {
             self::fail();
         }, $acquireTimeout);
@@ -78,10 +77,10 @@ public function testExecutionWithinAcquireTimeout(): void
 
     public function testExecutionWithinAcquireTimeoutWithoutCallingEnd(): void
     {
+        $loop = new Loop();
+
         $this->expectException(LockAcquireTimeoutException::class);
         $this->expectExceptionMessage('Lock acquire timeout of 0.5 seconds has been exceeded');
-
-        $loop = new Loop();
         $loop->execute(static function () {
             usleep(10 * 1000);
         }, 0.5);
@@ -102,20 +101,20 @@ public function testExceedAcquireTimeoutIsAcceptableIfEndWasCalled(): void
 
     public function testExceedAcquireTimeoutWithoutCallingEnd(): void
     {
+        $loop = new Loop();
+
         $this->expectException(LockAcquireTimeoutException::class);
         $this->expectExceptionMessage('Lock acquire timeout of 0.5 seconds has been exceeded');
-
-        $loop = new Loop();
         $loop->execute(static function () {
             usleep(501 * 1000);
         }, 0.5);
     }
 
     public function testExceptionStopsIteration(): void
     {
-        $this->expectException(\DomainException::class);
-
         $loop = new Loop();
+
+        $this->expectException(\DomainException::class);
         $loop->execute(static function () {
             throw new \DomainException();
         }, 1);

tests/Util/PcntlTimeoutTest.php

@@ -21,10 +21,9 @@ class PcntlTimeoutTest extends TestCase
      */
     public function testShouldTimeout(): void
     {
-        $this->expectException(DeadlineException::class);
-
         $timeout = new PcntlTimeout(1);
 
+        $this->expectException(DeadlineException::class);
         $timeout->timeBoxed(static function () {
             sleep(2);
         });
@@ -44,22 +43,37 @@ public function testShouldNotTimeout(): void
         self::assertSame(42, $result);
     }
 
+    /**
+     * Thrown exceptions from the subject code should be rethrown.
+     */
+    public function testShouldThrowException(): void
+    {
+        $timeout = new PcntlTimeout(1);
+
+        $this->expectException(\DomainException::class);
+        $timeout->timeBoxed(static function () {
+            throw new \DomainException();
+        });
+    }
+
     /**
      * When a previous scheduled alarm exists, it should fail.
      */
     public function testShouldFailOnExistingAlarm(): void
     {
-        $this->expectException(LockAcquireException::class);
-
+        $origSignalHandler = pcntl_signal_get_handler(\SIGALRM);
         try {
             pcntl_alarm(1);
             $timeout = new PcntlTimeout(1);
 
+            $this->expectException(LockAcquireException::class);
+            $this->expectExceptionMessage('Existing process alarm is not supported');
             $timeout->timeBoxed(static function () {
                 sleep(1);
             });
         } finally {
             pcntl_alarm(0);
+            self::assertSame($origSignalHandler, pcntl_signal_get_handler(\SIGALRM));
         }
     }
 
@@ -74,4 +88,21 @@ public function testShouldResetAlarmWhenNotTimeout(): void
 
         self::assertSame(0, pcntl_alarm(0));
     }
+
+    /**
+     * After not timing out and throwing an exception, there should be no alarm scheduled.
+     */
+    public function testShouldResetAlarmWhenNotTimeoutAndException(): void
+    {
+        $timeout = new PcntlTimeout(3);
+
+        $this->expectException(\DomainException::class);
+        try {
+            $timeout->timeBoxed(static function () {
+                throw new \DomainException();
+            });
+        } finally {
+            self::assertSame(0, pcntl_alarm(0));
+        }
+    }
 }