From 56f3100214d1274e876f657632913f397bbfd04d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Fri, 10 Nov 2017 17:05:37 +0100 Subject: [PATCH 1/2] Replace internal removeStream() with removeReadStream() --- src/DuplexResourceStream.php | 14 ++++---------- src/ReadableResourceStream.php | 14 ++++---------- tests/DuplexResourceStreamTest.php | 17 ++++++++++++++++- tests/ReadableResourceStreamTest.php | 14 ++++++++++++++ 4 files changed, 38 insertions(+), 21 deletions(-) diff --git a/src/DuplexResourceStream.php b/src/DuplexResourceStream.php index 7869eda..92e29b5 100644 --- a/src/DuplexResourceStream.php +++ b/src/DuplexResourceStream.php @@ -131,11 +131,13 @@ public function close() $this->writable = false; $this->emit('close'); - $this->loop->removeStream($this->stream); + $this->pause(); $this->buffer->close(); $this->removeAllListeners(); - $this->handleClose(); + if (is_resource($this->stream)) { + fclose($this->stream); + } } public function end($data = null) @@ -191,14 +193,6 @@ public function handleData($stream) } } - /** @internal */ - public function handleClose() - { - if (is_resource($this->stream)) { - fclose($this->stream); - } - } - /** * Returns whether this is a pipe resource in a legacy environment * diff --git a/src/ReadableResourceStream.php b/src/ReadableResourceStream.php index 6a9cd65..d61e178 100644 --- a/src/ReadableResourceStream.php +++ b/src/ReadableResourceStream.php @@ -105,10 +105,12 @@ public function close() $this->closed = true; $this->emit('close'); - $this->loop->removeStream($this->stream); + $this->pause(); $this->removeAllListeners(); - $this->handleClose(); + if (is_resource($this->stream)) { + fclose($this->stream); + } } /** @internal */ @@ -144,14 +146,6 @@ public function handleData() } } - /** @internal */ - public function handleClose() - { - if (is_resource($this->stream)) { - fclose($this->stream); - } - } - /** * Returns whether this is a pipe resource in a legacy environment * diff --git a/tests/DuplexResourceStreamTest.php b/tests/DuplexResourceStreamTest.php index 9ea43f9..402f85d 100644 --- a/tests/DuplexResourceStreamTest.php +++ b/tests/DuplexResourceStreamTest.php @@ -242,12 +242,27 @@ public function testEndRemovesReadStreamFromLoop() { $stream = fopen('php://temp', 'r+'); $loop = $this->createLoopMock(); - $loop->expects($this->once())->method('removeReadStream'); + $loop->expects($this->once())->method('addReadStream')->with($stream); + $loop->expects($this->once())->method('removeReadStream')->with($stream); $conn = new DuplexResourceStream($stream, $loop); $conn->end('bye'); } + /** + * @covers React\Stream\DuplexResourceStream::close + */ + public function testCloseRemovesReadStreamFromLoop() + { + $stream = fopen('php://temp', 'r+'); + $loop = $this->createLoopMock(); + $loop->expects($this->once())->method('addReadStream')->with($stream); + $loop->expects($this->once())->method('removeReadStream')->with($stream); + + $conn = new DuplexResourceStream($stream, $loop); + $conn->close(); + } + public function testEndedStreamsShouldNotWrite() { $file = tempnam(sys_get_temp_dir(), 'reactphptest_'); diff --git a/tests/ReadableResourceStreamTest.php b/tests/ReadableResourceStreamTest.php index a6909ba..5be1e15 100644 --- a/tests/ReadableResourceStreamTest.php +++ b/tests/ReadableResourceStreamTest.php @@ -204,6 +204,20 @@ public function testClosingStreamInDataEventShouldNotTriggerError() $conn->handleData($stream); } + /** + * @covers React\Stream\ReadableResourceStream::close + */ + public function testCloseRemovesReadStreamFromLoop() + { + $stream = fopen('php://temp', 'r+'); + $loop = $this->createLoopMock(); + $loop->expects($this->once())->method('addReadStream')->with($stream); + $loop->expects($this->once())->method('removeReadStream')->with($stream); + + $conn = new ReadableResourceStream($stream, $loop); + $conn->close(); + } + /** * @covers React\Stream\ReadableResourceStream::handleData */ From cf4ace0df32fdc1124090503789fba9380621929 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Fri, 10 Nov 2017 17:46:22 +0100 Subject: [PATCH 2/2] Only remove stream from loop if it was actually added --- src/DuplexResourceStream.php | 9 ++++- src/ReadableResourceStream.php | 9 ++++- tests/DuplexResourceStreamTest.php | 58 ++++++++++++++++++++++++++++ tests/ReadableResourceStreamTest.php | 58 ++++++++++++++++++++++++++++ 4 files changed, 130 insertions(+), 4 deletions(-) diff --git a/src/DuplexResourceStream.php b/src/DuplexResourceStream.php index 92e29b5..46f7a21 100644 --- a/src/DuplexResourceStream.php +++ b/src/DuplexResourceStream.php @@ -33,6 +33,7 @@ final class DuplexResourceStream extends EventEmitter implements DuplexStreamInt private $readable = true; private $writable = true; private $closing = false; + private $listening = false; public function __construct($stream, LoopInterface $loop, $readChunkSize = null, WritableStreamInterface $buffer = null) { @@ -100,13 +101,17 @@ public function isWritable() public function pause() { - $this->loop->removeReadStream($this->stream); + if ($this->listening) { + $this->loop->removeReadStream($this->stream); + $this->listening = false; + } } public function resume() { - if ($this->readable) { + if (!$this->listening && $this->readable) { $this->loop->addReadStream($this->stream, array($this, 'handleData')); + $this->listening = true; } } diff --git a/src/ReadableResourceStream.php b/src/ReadableResourceStream.php index d61e178..a0a0339 100644 --- a/src/ReadableResourceStream.php +++ b/src/ReadableResourceStream.php @@ -36,6 +36,7 @@ final class ReadableResourceStream extends EventEmitter implements ReadableStrea private $bufferSize; private $closed = false; + private $listening = false; public function __construct($stream, LoopInterface $loop, $readChunkSize = null) { @@ -81,13 +82,17 @@ public function isReadable() public function pause() { - $this->loop->removeReadStream($this->stream); + if ($this->listening) { + $this->loop->removeReadStream($this->stream); + $this->listening = false; + } } public function resume() { - if (!$this->closed) { + if (!$this->listening && !$this->closed) { $this->loop->addReadStream($this->stream, array($this, 'handleData')); + $this->listening = true; } } diff --git a/tests/DuplexResourceStreamTest.php b/tests/DuplexResourceStreamTest.php index 402f85d..6b63e4c 100644 --- a/tests/DuplexResourceStreamTest.php +++ b/tests/DuplexResourceStreamTest.php @@ -249,6 +249,35 @@ public function testEndRemovesReadStreamFromLoop() $conn->end('bye'); } + /** + * @covers React\Stream\DuplexResourceStream::pause + */ + public function testPauseRemovesReadStreamFromLoop() + { + $stream = fopen('php://temp', 'r+'); + $loop = $this->createLoopMock(); + $loop->expects($this->once())->method('addReadStream')->with($stream); + $loop->expects($this->once())->method('removeReadStream')->with($stream); + + $conn = new DuplexResourceStream($stream, $loop); + $conn->pause(); + $conn->pause(); + } + + /** + * @covers React\Stream\DuplexResourceStream::pause + */ + public function testResumeDoesAddStreamToLoopOnlyOnce() + { + $stream = fopen('php://temp', 'r+'); + $loop = $this->createLoopMock(); + $loop->expects($this->once())->method('addReadStream')->with($stream); + + $conn = new DuplexResourceStream($stream, $loop); + $conn->resume(); + $conn->resume(); + } + /** * @covers React\Stream\DuplexResourceStream::close */ @@ -263,6 +292,35 @@ public function testCloseRemovesReadStreamFromLoop() $conn->close(); } + /** + * @covers React\Stream\DuplexResourceStream::close + */ + public function testCloseAfterPauseRemovesReadStreamFromLoopOnlyOnce() + { + $stream = fopen('php://temp', 'r+'); + $loop = $this->createLoopMock(); + $loop->expects($this->once())->method('addReadStream')->with($stream); + $loop->expects($this->once())->method('removeReadStream')->with($stream); + + $conn = new DuplexResourceStream($stream, $loop); + $conn->pause(); + $conn->close(); + } + + /** + * @covers React\Stream\DuplexResourceStream::close + */ + public function testResumeAfterCloseDoesAddReadStreamToLoopOnlyOnce() + { + $stream = fopen('php://temp', 'r+'); + $loop = $this->createLoopMock(); + $loop->expects($this->once())->method('addReadStream')->with($stream); + + $conn = new DuplexResourceStream($stream, $loop); + $conn->close(); + $conn->resume(); + } + public function testEndedStreamsShouldNotWrite() { $file = tempnam(sys_get_temp_dir(), 'reactphptest_'); diff --git a/tests/ReadableResourceStreamTest.php b/tests/ReadableResourceStreamTest.php index 5be1e15..71bbba0 100644 --- a/tests/ReadableResourceStreamTest.php +++ b/tests/ReadableResourceStreamTest.php @@ -204,6 +204,35 @@ public function testClosingStreamInDataEventShouldNotTriggerError() $conn->handleData($stream); } + /** + * @covers React\Stream\ReadableResourceStream::pause + */ + public function testPauseRemovesReadStreamFromLoop() + { + $stream = fopen('php://temp', 'r+'); + $loop = $this->createLoopMock(); + $loop->expects($this->once())->method('addReadStream')->with($stream); + $loop->expects($this->once())->method('removeReadStream')->with($stream); + + $conn = new ReadableResourceStream($stream, $loop); + $conn->pause(); + $conn->pause(); + } + + /** + * @covers React\Stream\ReadableResourceStream::pause + */ + public function testResumeDoesAddStreamToLoopOnlyOnce() + { + $stream = fopen('php://temp', 'r+'); + $loop = $this->createLoopMock(); + $loop->expects($this->once())->method('addReadStream')->with($stream); + + $conn = new ReadableResourceStream($stream, $loop); + $conn->resume(); + $conn->resume(); + } + /** * @covers React\Stream\ReadableResourceStream::close */ @@ -218,6 +247,35 @@ public function testCloseRemovesReadStreamFromLoop() $conn->close(); } + /** + * @covers React\Stream\ReadableResourceStream::close + */ + public function testCloseAfterPauseRemovesReadStreamFromLoopOnce() + { + $stream = fopen('php://temp', 'r+'); + $loop = $this->createLoopMock(); + $loop->expects($this->once())->method('addReadStream')->with($stream); + $loop->expects($this->once())->method('removeReadStream')->with($stream); + + $conn = new ReadableResourceStream($stream, $loop); + $conn->pause(); + $conn->close(); + } + + /** + * @covers React\Stream\ReadableResourceStream::close + */ + public function testResumeAfterCloseDoesAddReadStreamToLoopOnlyOnce() + { + $stream = fopen('php://temp', 'r+'); + $loop = $this->createLoopMock(); + $loop->expects($this->once())->method('addReadStream')->with($stream); + + $conn = new ReadableResourceStream($stream, $loop); + $conn->close(); + $conn->resume(); + } + /** * @covers React\Stream\ReadableResourceStream::handleData */