Skip to content

Commit 14c870e

Browse files
jsorjsor
authored
Merge pull request #150 from clue-labs/signals
Refactor and simplify signal handling logic
2 parents e03f114 + f261f47 commit 14c870e

File tree

7 files changed

+70
-165
lines changed

7 files changed

+70
-165
lines changed

‎src/ExtEventLoop.php

Lines changed: 12 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -41,27 +41,7 @@ public function __construct(EventBaseConfig $config = null)
4141
$this->eventBase = new EventBase($config);
4242
$this->futureTickQueue = new FutureTickQueue();
4343
$this->timerEvents = new SplObjectStorage();
44-
45-
$this->signals = new SignalsHandler(
46-
$this,
47-
function ($signal) {
48-
$this->signalEvents[$signal] = Event::signal($this->eventBase, $signal, $f = function () use ($signal, &$f) {
49-
$this->signals->call($signal);
50-
// Ensure there are two copies of the callable around until it has been executed.
51-
// For more information see: https://bugs.php.net/bug.php?id=62452
52-
// Only an issue for PHP 5, this hack can be removed once PHP 5 support has been dropped.
53-
$g = $f;
54-
$f = $g;
55-
});
56-
$this->signalEvents[$signal]->add();
57-
},
58-
function ($signal) {
59-
if ($this->signals->count($signal) === 0) {
60-
$this->signalEvents[$signal]->free();
61-
unset($this->signalEvents[$signal]);
62-
}
63-
}
64-
);
44+
$this->signals = new SignalsHandler();
6545

6646
$this->createTimerCallback();
6747
$this->createStreamCallback();
@@ -167,11 +147,21 @@ public function futureTick($listener)
167147
public function addSignal($signal, $listener)
168148
{
169149
$this->signals->add($signal, $listener);
150+
151+
if (!isset($this->signalEvents[$signal])) {
152+
$this->signalEvents[$signal] = Event::signal($this->eventBase, $signal, array($this->signals, 'call'));
153+
$this->signalEvents[$signal]->add();
154+
}
170155
}
171156

172157
public function removeSignal($signal, $listener)
173158
{
174159
$this->signals->remove($signal, $listener);
160+
161+
if (isset($this->signalEvents[$signal]) && $this->signals->count($signal) === 0) {
162+
$this->signalEvents[$signal]->free();
163+
unset($this->signalEvents[$signal]);
164+
}
175165
}
176166

177167
public function run()
@@ -184,7 +174,7 @@ public function run()
184174
$flags = EventBase::LOOP_ONCE;
185175
if (!$this->running || !$this->futureTickQueue->isEmpty()) {
186176
$flags |= EventBase::LOOP_NONBLOCK;
187-
} elseif (!$this->readEvents && !$this->writeEvents && !$this->timerEvents->count()) {
177+
} elseif (!$this->readEvents && !$this->writeEvents && !$this->timerEvents->count() && $this->signals->isEmpty()) {
188178
break;
189179
}
190180

‎src/ExtLibevLoop.php

Lines changed: 15 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -39,28 +39,7 @@ public function __construct()
3939
$this->loop = new EventLoop();
4040
$this->futureTickQueue = new FutureTickQueue();
4141
$this->timerEvents = new SplObjectStorage();
42-
43-
$this->signals = new SignalsHandler(
44-
$this,
45-
function ($signal) {
46-
$this->signalEvents[$signal] = new SignalEvent($f = function () use ($signal, &$f) {
47-
$this->signals->call($signal);
48-
// Ensure there are two copies of the callable around until it has been executed.
49-
// For more information see: https://bugs.php.net/bug.php?id=62452
50-
// Only an issue for PHP 5, this hack can be removed once PHP 5 support has been dropped.
51-
$g = $f;
52-
$f = $g;
53-
}, $signal);
54-
$this->loop->add($this->signalEvents[$signal]);
55-
},
56-
function ($signal) {
57-
if ($this->signals->count($signal) === 0) {
58-
$this->signalEvents[$signal]->stop();
59-
$this->loop->remove($this->signalEvents[$signal]);
60-
unset($this->signalEvents[$signal]);
61-
}
62-
}
63-
);
42+
$this->signals = new SignalsHandler();
6443
}
6544

6645
public function addReadStream($stream, $listener)
@@ -167,11 +146,24 @@ public function futureTick($listener)
167146
public function addSignal($signal, $listener)
168147
{
169148
$this->signals->add($signal, $listener);
149+
150+
if (!isset($this->signalEvents[$signal])) {
151+
$this->signalEvents[$signal] = new SignalEvent(function () use ($signal) {
152+
$this->signals->call($signal);
153+
}, $signal);
154+
$this->loop->add($this->signalEvents[$signal]);
155+
}
170156
}
171157

172158
public function removeSignal($signal, $listener)
173159
{
174160
$this->signals->remove($signal, $listener);
161+
162+
if (isset($this->signalEvents[$signal]) && $this->signals->count($signal) === 0) {
163+
$this->signalEvents[$signal]->stop();
164+
$this->loop->remove($this->signalEvents[$signal]);
165+
unset($this->signalEvents[$signal]);
166+
}
175167
}
176168

177169
public function run()
@@ -184,7 +176,7 @@ public function run()
184176
$flags = EventLoop::RUN_ONCE;
185177
if (!$this->running || !$this->futureTickQueue->isEmpty()) {
186178
$flags |= EventLoop::RUN_NOWAIT;
187-
} elseif (!$this->readEvents && !$this->writeEvents && !$this->timerEvents->count()) {
179+
} elseif (!$this->readEvents && !$this->writeEvents && !$this->timerEvents->count() && $this->signals->isEmpty()) {
188180
break;
189181
}
190182

‎src/ExtLibeventLoop.php

Lines changed: 15 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -55,30 +55,7 @@ public function __construct()
5555
$this->eventBase = event_base_new();
5656
$this->futureTickQueue = new FutureTickQueue();
5757
$this->timerEvents = new SplObjectStorage();
58-
59-
$this->signals = new SignalsHandler(
60-
$this,
61-
function ($signal) {
62-
$this->signalEvents[$signal] = event_new();
63-
event_set($this->signalEvents[$signal], $signal, EV_PERSIST | EV_SIGNAL, $f = function () use ($signal, &$f) {
64-
$this->signals->call($signal);
65-
// Ensure there are two copies of the callable around until it has been executed.
66-
// For more information see: https://bugs.php.net/bug.php?id=62452
67-
// Only an issue for PHP 5, this hack can be removed once PHP 5 support has been dropped.
68-
$g = $f;
69-
$f = $g;
70-
});
71-
event_base_set($this->signalEvents[$signal], $this->eventBase);
72-
event_add($this->signalEvents[$signal]);
73-
},
74-
function ($signal) {
75-
if ($this->signals->count($signal) === 0) {
76-
event_del($this->signalEvents[$signal]);
77-
event_free($this->signalEvents[$signal]);
78-
unset($this->signalEvents[$signal]);
79-
}
80-
}
81-
);
58+
$this->signals = new SignalsHandler();
8259

8360
$this->createTimerCallback();
8461
$this->createStreamCallback();
@@ -185,11 +162,24 @@ public function futureTick($listener)
185162
public function addSignal($signal, $listener)
186163
{
187164
$this->signals->add($signal, $listener);
165+
166+
if (!isset($this->signalEvents[$signal])) {
167+
$this->signalEvents[$signal] = event_new();
168+
event_set($this->signalEvents[$signal], $signal, EV_PERSIST | EV_SIGNAL, array($this->signals, 'call'));
169+
event_base_set($this->signalEvents[$signal], $this->eventBase);
170+
event_add($this->signalEvents[$signal]);
171+
}
188172
}
189173

190174
public function removeSignal($signal, $listener)
191175
{
192176
$this->signals->remove($signal, $listener);
177+
178+
if (isset($this->signalEvents[$signal]) && $this->signals->count($signal) === 0) {
179+
event_del($this->signalEvents[$signal]);
180+
event_free($this->signalEvents[$signal]);
181+
unset($this->signalEvents[$signal]);
182+
}
193183
}
194184

195185
public function run()
@@ -202,7 +192,7 @@ public function run()
202192
$flags = EVLOOP_ONCE;
203193
if (!$this->running || !$this->futureTickQueue->isEmpty()) {
204194
$flags |= EVLOOP_NONBLOCK;
205-
} elseif (!$this->readEvents && !$this->writeEvents && !$this->timerEvents->count()) {
195+
} elseif (!$this->readEvents && !$this->writeEvents && !$this->timerEvents->count() && $this->signals->isEmpty()) {
206196
break;
207197
}
208198

‎src/SignalsHandler.php

Lines changed: 5 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -7,41 +7,12 @@
77
*/
88
final class SignalsHandler
99
{
10-
private $loop;
11-
private $timer;
1210
private $signals = [];
13-
private $on;
14-
private $off;
15-
16-
public function __construct(LoopInterface $loop, $on, $off)
17-
{
18-
$this->loop = $loop;
19-
$this->on = $on;
20-
$this->off = $off;
21-
}
22-
23-
public function __destruct()
24-
{
25-
$off = $this->off;
26-
foreach ($this->signals as $signal => $listeners) {
27-
$off($signal);
28-
}
29-
}
3011

3112
public function add($signal, $listener)
3213
{
33-
if (empty($this->signals) && $this->timer === null) {
34-
/**
35-
* Timer to keep the loop alive as long as there are any signal handlers registered
36-
*/
37-
$this->timer = $this->loop->addPeriodicTimer(300, function () {});
38-
}
39-
4014
if (!isset($this->signals[$signal])) {
4115
$this->signals[$signal] = [];
42-
43-
$on = $this->on;
44-
$on($signal);
4516
}
4617

4718
if (in_array($listener, $this->signals[$signal])) {
@@ -62,14 +33,6 @@ public function remove($signal, $listener)
6233

6334
if (isset($this->signals[$signal]) && \count($this->signals[$signal]) === 0) {
6435
unset($this->signals[$signal]);
65-
66-
$off = $this->off;
67-
$off($signal);
68-
}
69-
70-
if (empty($this->signals) && $this->timer instanceof TimerInterface) {
71-
$this->loop->cancelTimer($this->timer);
72-
$this->timer = null;
7336
}
7437
}
7538

@@ -92,4 +55,9 @@ public function count($signal)
9255

9356
return \count($this->signals[$signal]);
9457
}
58+
59+
public function isEmpty()
60+
{
61+
return !$this->signals;
62+
}
9563
}

‎src/StreamSelectLoop.php

Lines changed: 16 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -69,24 +69,7 @@ public function __construct()
6969
$this->futureTickQueue = new FutureTickQueue();
7070
$this->timers = new Timers();
7171
$this->pcntl = extension_loaded('pcntl');
72-
$this->signals = new SignalsHandler(
73-
$this,
74-
function ($signal) {
75-
\pcntl_signal($signal, $f = function ($signal) use (&$f) {
76-
$this->signals->call($signal);
77-
// Ensure there are two copies of the callable around until it has been executed.
78-
// For more information see: https://bugs.php.net/bug.php?id=62452
79-
// Only an issue for PHP 5, this hack can be removed once PHP 5 support has been dropped.
80-
$g = $f;
81-
$f = $g;
82-
});
83-
},
84-
function ($signal) {
85-
if ($this->signals->count($signal) === 0) {
86-
\pcntl_signal($signal, SIG_DFL);
87-
}
88-
}
89-
);
72+
$this->signals = new SignalsHandler();
9073
}
9174

9275
public function addReadStream($stream, $listener)
@@ -163,12 +146,25 @@ public function addSignal($signal, $listener)
163146
throw new \BadMethodCallException('Event loop feature "signals" isn\'t supported by the "StreamSelectLoop"');
164147
}
165148

149+
$first = $this->signals->count($signal) === 0;
166150
$this->signals->add($signal, $listener);
151+
152+
if ($first) {
153+
\pcntl_signal($signal, array($this->signals, 'call'));
154+
}
167155
}
168156

169157
public function removeSignal($signal, $listener)
170158
{
159+
if (!$this->signals->count($signal)) {
160+
return;
161+
}
162+
171163
$this->signals->remove($signal, $listener);
164+
165+
if ($this->signals->count($signal) === 0) {
166+
\pcntl_signal($signal, SIG_DFL);
167+
}
172168
}
173169

174170
public function run()
@@ -197,8 +193,8 @@ public function run()
197193
$timeout = $timeout > PHP_INT_MAX ? PHP_INT_MAX : (int)$timeout;
198194
}
199195

200-
// The only possible event is stream activity, so wait forever ...
201-
} elseif ($this->readStreams || $this->writeStreams) {
196+
// The only possible event is stream or signal activity, so wait forever ...
197+
} elseif ($this->readStreams || $this->writeStreams || !$this->signals->isEmpty()) {
202198
$timeout = null;
203199

204200
// There's nothing left to do ...

‎tests/AbstractLoopTest.php

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -475,6 +475,12 @@ function () {
475475
$this->loop->run();
476476
}
477477

478+
public function testRemoveSignalNotRegisteredIsNoOp()
479+
{
480+
$this->loop->removeSignal(SIGINT, function () { });
481+
$this->assertTrue(true);
482+
}
483+
478484
public function testSignal()
479485
{
480486
if (!function_exists('posix_kill') || !function_exists('posix_getpid')) {

0 commit comments

Comments
 (0)