From a13bb6f7212b91abff13e58da4dfa32e22839ac1 Mon Sep 17 00:00:00 2001 From: Niklas Keller Date: Wed, 21 Jun 2017 11:01:19 +0200 Subject: [PATCH] Use ClosedException as mandated by the ByteStream interfaces This commit also changes end() to wait for the close() to finish before returning. --- lib/BlockingHandle.php | 17 ++++++++++++----- lib/EioHandle.php | 22 ++++++++++++++-------- lib/FilesystemException.php | 4 +++- lib/ParallelHandle.php | 23 +++++++++++++++-------- lib/UvHandle.php | 24 ++++++++++++++---------- test/HandleTest.php | 33 ++++++++++++++++++++++++++++++--- 6 files changed, 88 insertions(+), 35 deletions(-) diff --git a/lib/BlockingHandle.php b/lib/BlockingHandle.php index 1ca7e8c..ed01080 100644 --- a/lib/BlockingHandle.php +++ b/lib/BlockingHandle.php @@ -2,9 +2,11 @@ namespace Amp\File; +use Amp\ByteStream\ClosedException; use Amp\Failure; use Amp\Promise; use Amp\Success; +use function Amp\call; class BlockingHandle implements Handle { private $fh; @@ -33,7 +35,7 @@ class BlockingHandle implements Handle { */ public function read(int $length = self::DEFAULT_READ_LENGTH): Promise { if ($this->fh === null) { - throw new FilesystemException("The file has been closed"); + throw new ClosedException("The file has been closed"); } $data = \fread($this->fh, $length); @@ -52,7 +54,7 @@ class BlockingHandle implements Handle { */ public function write(string $data): Promise { if ($this->fh === null) { - throw new FilesystemException("The file has been closed"); + throw new ClosedException("The file has been closed"); } $len = \fwrite($this->fh, $data); @@ -70,9 +72,14 @@ class BlockingHandle implements Handle { * {@inheritdoc} */ public function end(string $data = ""): Promise { - $promise = $this->write($data); - $promise->onResolve([$this, "close"]); - return $promise; + return call(function () use ($data) { + $promise = $this->write($data); + + // ignore any errors + yield Promise\any([$this->close()]); + + return $promise; + }); } /** diff --git a/lib/EioHandle.php b/lib/EioHandle.php index 94b2ffe..31d6fc4 100644 --- a/lib/EioHandle.php +++ b/lib/EioHandle.php @@ -2,6 +2,7 @@ namespace Amp\File; +use Amp\ByteStream\ClosedException; use Amp\Deferred; use Amp\Promise; use Amp\Success; @@ -61,7 +62,7 @@ class EioHandle implements Handle { $this->isActive = false; if ($result === -1) { - $deferred->fail(new FilesystemException( + $deferred->fail(new ClosedException( sprintf('Reading from file failed: %s.', \eio_get_last_error($req)) )); } else { @@ -79,7 +80,7 @@ class EioHandle implements Handle { } if (!$this->writable) { - throw new \Error("The file is no longer writable"); + throw new ClosedException("The file is no longer writable"); } $this->isActive = true; @@ -122,15 +123,20 @@ class EioHandle implements Handle { * {@inheritdoc} */ public function end(string $data = ""): Promise { - $promise = $this->write($data); - $this->writable = false; - $promise->onResolve([$this, "close"]); - return $promise; + return call(function () use ($data) { + $promise = $this->write($data); + $this->writable = false; + + // ignore any errors + yield Promise\any([$this->close()]); + + return $promise; + }); } private function onWrite(Deferred $deferred, $result, $req) { if ($this->queue->isEmpty()) { - $deferred->fail(new FilesystemException('No pending write, the file may have been closed')); + $deferred->fail(new ClosedException('No pending write, the file may have been closed')); } $this->queue->shift(); @@ -139,7 +145,7 @@ class EioHandle implements Handle { } if ($result === -1) { - $deferred->fail(new FilesystemException( + $deferred->fail(new ClosedException( sprintf('Writing to the file failed: %s', \eio_get_last_error($req)) )); } else { diff --git a/lib/FilesystemException.php b/lib/FilesystemException.php index 7b36a81..195b82b 100644 --- a/lib/FilesystemException.php +++ b/lib/FilesystemException.php @@ -2,7 +2,9 @@ namespace Amp\File; -class FilesystemException extends \Exception { +use Amp\ByteStream\StreamException; + +class FilesystemException extends StreamException { public function __construct(string $message, \Throwable $previous = null) { parent::__construct($message, 0, $previous); } diff --git a/lib/ParallelHandle.php b/lib/ParallelHandle.php index 74908ef..f7a8b6a 100644 --- a/lib/ParallelHandle.php +++ b/lib/ParallelHandle.php @@ -2,12 +2,14 @@ namespace Amp\File; +use Amp\ByteStream\ClosedException; use Amp\Coroutine; use Amp\Parallel\Worker\TaskException; use Amp\Parallel\Worker\Worker; use Amp\Parallel\Worker\WorkerException; use Amp\Promise; use Amp\Success; +use function Amp\call; class ParallelHandle implements Handle { /** @var \Amp\Parallel\Worker\Worker */ @@ -90,7 +92,7 @@ class ParallelHandle implements Handle { public function read(int $length = self::DEFAULT_READ_LENGTH): Promise { if ($this->id === null) { - throw new FilesystemException("The file has been closed"); + throw new ClosedException("The file has been closed"); } if ($this->busy) { @@ -121,7 +123,7 @@ class ParallelHandle implements Handle { */ public function write(string $data): Promise { if ($this->id === null) { - throw new FilesystemException("The file has been closed"); + throw new ClosedException("The file has been closed"); } if ($this->busy && $this->pendingWrites === 0) { @@ -129,7 +131,7 @@ class ParallelHandle implements Handle { } if (!$this->writable) { - throw new \Error("The file is no longer writable"); + throw new ClosedException("The file is no longer writable"); } return new Coroutine($this->doWrite($data)); @@ -139,10 +141,15 @@ class ParallelHandle implements Handle { * {@inheritdoc} */ public function end(string $data = ""): Promise { - $promise = $this->write($data); - $this->writable = false; - $promise->onResolve([$this, "close"]); - return $promise; + return call(function () use ($data) { + $promise = $this->write($data); + $this->writable = false; + + // ignore any errors + yield Promise\any([$this->close()]); + + return $promise; + }); } private function doWrite(string $data): \Generator { @@ -170,7 +177,7 @@ class ParallelHandle implements Handle { */ public function seek(int $offset, int $whence = SEEK_SET): Promise { if ($this->id === null) { - throw new FilesystemException("The file has been closed"); + throw new ClosedException("The file has been closed"); } if ($this->busy) { diff --git a/lib/UvHandle.php b/lib/UvHandle.php index f404cb8..10c2a7e 100644 --- a/lib/UvHandle.php +++ b/lib/UvHandle.php @@ -2,6 +2,7 @@ namespace Amp\File; +use Amp\ByteStream\ClosedException; use Amp\Deferred; use Amp\File\Internal\UvPoll; use Amp\Loop; @@ -57,7 +58,7 @@ class UvHandle implements Handle { $this->isActive = false; if ($result < 0) { - $deferred->fail(new FilesystemException(\uv_strerror($result))); + $deferred->fail(new ClosedException(\uv_strerror($result))); } else { $length = strlen($buffer); $this->position = $this->position + $length; @@ -79,7 +80,7 @@ class UvHandle implements Handle { } if (!$this->writable) { - throw new \Error("The file is no longer writable"); + throw new ClosedException("The file is no longer writable"); } $this->isActive = true; @@ -103,10 +104,15 @@ class UvHandle implements Handle { * {@inheritdoc} */ public function end(string $data = ""): Promise { - $promise = $this->write($data); - $this->writable = false; - $promise->onResolve([$this, "close"]); - return $promise; + return call(function () use ($data) { + $promise = $this->write($data); + $this->writable = false; + + // ignore any errors + yield Promise\any([$this->close()]); + + return $promise; + }); } private function push(string $data): Promise { @@ -117,7 +123,7 @@ class UvHandle implements Handle { $onWrite = function ($fh, $result) use ($deferred, $length) { if ($this->queue->isEmpty()) { - $deferred->fail(new FilesystemException('No pending write, the file may have been closed')); + $deferred->fail(new ClosedException('No pending write, the file may have been closed')); } $this->queue->shift(); @@ -126,9 +132,7 @@ class UvHandle implements Handle { } if ($result < 0) { - $deferred->fail(new FilesystemException( - \uv_strerror($result) - )); + $deferred->fail(new ClosedException(\uv_strerror($result))); } else { StatCache::clear($this->path); $newPosition = $this->position + $length; diff --git a/test/HandleTest.php b/test/HandleTest.php index adf8c83..9d9c5a0 100644 --- a/test/HandleTest.php +++ b/test/HandleTest.php @@ -2,6 +2,7 @@ namespace Amp\File\Test; +use Amp\ByteStream\ClosedException; use Amp\File; use Amp\PHPUnit\TestCase; @@ -36,6 +37,33 @@ abstract class HandleTest extends TestCase { }); } + public function testWriteAfterClose() { + $this->execute(function () { + $path = Fixture::path() . "/write"; + /** @var \Amp\File\Handle $handle */ + $handle = yield File\open($path, "c+"); + $this->assertSame(0, $handle->tell()); + yield $handle->write("foo"); + yield $handle->close(); + + $this->expectException(ClosedException::class); + yield $handle->write("bar"); + }); + } + + public function testWriteAfterEnd() { + $this->execute(function () { + $path = Fixture::path() . "/write"; + /** @var \Amp\File\Handle $handle */ + $handle = yield File\open($path, "c+"); + $this->assertSame(0, $handle->tell()); + yield $handle->end("foo"); + + $this->expectException(ClosedException::class); + yield $handle->write("bar"); + }); + } + public function testReadingToEof() { $this->execute(function () { /** @var \Amp\File\Handle $handle */ @@ -150,14 +178,13 @@ abstract class HandleTest extends TestCase { }); } - /** - * @expectedException \Amp\File\FilesystemException - */ public function testClose() { $this->execute(function () { /** @var \Amp\File\Handle $handle */ $handle = yield File\open(__FILE__, "r"); yield $handle->close(); + + $this->expectException(ClosedException::class); yield $handle->read(); }); }