chore(io): do not throw already closed exception when closing twice (#…

…274) Signed-off-by: azjezz <azjezz@protonmail.com>
azjezz · Nov 13, 2021 · 26dbdb9 · 26dbdb9
1 parent 1b372be
commit 26dbdb9
Show file tree

Hide file tree

Showing 7 changed files with 80 additions and 67 deletions.
diff --git a/src/Psl/File/Internal/AbstractHandleWrapper.php b/src/Psl/File/Internal/AbstractHandleWrapper.php
@@ -75,6 +75,6 @@ public function close(): void
      */
     public function getStream(): mixed
     {
-        $this->handle->getStream();
+        return $this->handle->getStream();
     }
 }
diff --git a/src/Psl/File/Internal/ResourceHandle.php b/src/Psl/File/Internal/ResourceHandle.php
@@ -30,11 +30,11 @@ final class ResourceHandle extends IO\Internal\ResourceHandle implements File\Re
     private string $path;
 
     /**
-     * @param resource|object $resource
+     * @param resource|object $stream
      */
-    public function __construct(string $path, mixed $resource, bool $read, bool $write)
+    public function __construct(string $path, mixed $stream, bool $read, bool $write)
     {
-        parent::__construct($resource, $read, $write, seek: true);
+        parent::__construct($stream, $read, $write, seek: true);
 
         $this->path = $path;
     }
@@ -52,7 +52,7 @@ public function getPath(): string
      */
     public function getSize(): int
     {
-        if (null === $this->resource) {
+        if (null === $this->stream) {
             throw new Exception\AlreadyClosedException('Handle has already been closed.');
         }
 
@@ -64,7 +64,7 @@ public function getSize(): int
         }
 
         /** @psalm-suppress PossiblyInvalidArgument */
-        $result = @fseek($this->resource, 0, SEEK_END);
+        $result = @fseek($this->stream, 0, SEEK_END);
         if ($result === -1) {
             $error = error_get_last();
 
@@ -103,13 +103,13 @@ public function lock(LockType $type): Lock
      */
     public function tryLock(LockType $type): Lock
     {
-        if (null === $this->resource) {
+        if (null === $this->stream) {
             throw new Exception\AlreadyClosedException('Handle has already been closed.');
         }
 
         $operations = LOCK_NB | ($type === LockType::EXCLUSIVE ? LOCK_EX : LOCK_SH);
         /** @psalm-suppress PossiblyInvalidArgument */
-        $success = @flock($this->resource, $operations, $would_block);
+        $success = @flock($this->stream, $operations, $would_block);
         // @codeCoverageIgnoreStart
         if ($would_block) {
             throw new File\Exception\AlreadyLockedException();
@@ -124,14 +124,14 @@ public function tryLock(LockType $type): Lock
         }
 
         return new Lock($type, function (): void {
-            if (null === $this->resource) {
+            if (null === $this->stream) {
                 // while closing a handle should unlock it, that is not always the case.
                 // therefore, we should require users to explicitly release the lock before closing the handle.
                 throw new Exception\AlreadyClosedException('Handle was closed before releasing the lock.');
             }
 
             /** @psalm-suppress PossiblyInvalidArgument */
-            if (!@flock($this->resource, LOCK_UN)) {
+            if (!@flock($this->stream, LOCK_UN)) {
                 throw new File\Exception\RuntimeException(Str\format(
                     'Could not release lock for "%s".',
                     $this->getPath(),

diff --git a/src/Psl/IO/CloseHandleInterface.php b/src/Psl/IO/CloseHandleInterface.php
@@ -12,7 +12,6 @@ interface CloseHandleInterface extends HandleInterface
     /**
      * Close the handle.
      *
-     * @throws Exception\AlreadyClosedException If the handle has been already closed.
      * @throws Exception\RuntimeException If unable to close the handle.
      */
     public function close(): void;

diff --git a/src/Psl/IO/Internal/ResourceHandle.php b/src/Psl/IO/Internal/ResourceHandle.php
@@ -17,6 +17,7 @@
 use function fseek;
 use function ftell;
 use function fwrite;
+use function is_resource;
 use function str_contains;
 use function stream_get_contents;
 use function stream_get_meta_data;
@@ -42,9 +43,9 @@ class ResourceHandle implements IO\Stream\CloseSeekReadWriteHandleInterface
     public const MAXIMUM_READ_BUFFER_SIZE = 786432;
 
     /**
-     * @var object|resource|null $resource
+     * @var object|resource|null $stream
      */
-    protected mixed $resource;
+    protected mixed $stream;
 
     private bool $useSingleRead;
 
@@ -57,20 +58,20 @@ class ResourceHandle implements IO\Stream\CloseSeekReadWriteHandleInterface
     private ?Suspension $writeSuspension = null;
 
     /**
-     * @param resource|object $resource
+     * @param resource|object $stream
      */
-    public function __construct(mixed $resource, bool $read, bool $write, bool $seek)
+    public function __construct(mixed $stream, bool $read, bool $write, bool $seek)
     {
-        $this->resource = Type\union(
+        $this->stream = Type\union(
             Type\resource('stream'),
             Type\object(),
-        )->assert($resource);
+        )->assert($stream);
 
         /** @psalm-suppress UnusedFunctionCall */
-        stream_set_read_buffer($resource, 0);
-        stream_set_blocking($resource, false);
+        stream_set_read_buffer($stream, 0);
+        stream_set_blocking($stream, false);
 
-        $meta = stream_get_meta_data($resource);
+        $meta = stream_get_meta_data($stream);
         $this->blocks = $meta['blocked'] || ($meta['wrapper_type'] ?? '') === 'plainfile';
         if ($seek) {
             $seekable = (bool)$meta['seekable'];
@@ -84,7 +85,7 @@ public function __construct(mixed $resource, bool $read, bool $write, bool $seek
             Psl\invariant($readable, 'Handle is not readable.');
 
             $suspension = &$this->readSuspension;
-            $this->readWatcher = EventLoop::onReadable($resource, static function () use (&$suspension) {
+            $this->readWatcher = EventLoop::onReadable($stream, static function () use (&$suspension) {
                 /** @var Suspension|null $suspension */
                 $suspension?->resume(null);
             });
@@ -102,7 +103,7 @@ public function __construct(mixed $resource, bool $read, bool $write, bool $seek
             Psl\invariant($writable, 'Handle is not writeable.');
 
             $suspension = &$this->writeSuspension;
-            $this->writeWatcher = EventLoop::onWritable($resource, static function () use (&$suspension) {
+            $this->writeWatcher = EventLoop::onWritable($stream, static function () use (&$suspension) {
                 /** @var Suspension|null $suspension */
                 $suspension?->resume(null);
             });
@@ -175,12 +176,12 @@ public function writeImmediately(string $bytes): int
             throw new Exception\RuntimeException('Pending operation.');
         }
 
-        if (null === $this->resource) {
+        if (!is_resource($this->stream)) {
             throw new Exception\AlreadyClosedException('Handle has already been closed.');
         }
 
         /** @psalm-suppress PossiblyInvalidArgument */
-        $result = @fwrite($this->resource, $bytes);
+        $result = @fwrite($this->stream, $bytes);
         if ($result === false) {
             $error = error_get_last();
 
@@ -196,14 +197,14 @@ public function writeImmediately(string $bytes): int
      */
     public function seek(int $offset): void
     {
-        if (null === $this->resource) {
+        if (!is_resource($this->stream)) {
             throw new Exception\AlreadyClosedException('Handle has already been closed.');
         }
 
         Psl\invariant($offset >= 0, '$offset must be a positive-int.');
 
         /** @psalm-suppress PossiblyInvalidArgument */
-        $result = @fseek($this->resource, $offset);
+        $result = @fseek($this->stream, $offset);
         if (0 !== $result) {
             throw new Exception\RuntimeException('Failed to seek the specified position.');
         }
@@ -215,12 +216,12 @@ public function seek(int $offset): void
      */
     public function tell(): int
     {
-        if (null === $this->resource) {
+        if (!is_resource($this->stream)) {
             throw new Exception\AlreadyClosedException('Handle has already been closed.');
         }
 
         /** @psalm-suppress PossiblyInvalidArgument */
-        $result = @ftell($this->resource);
+        $result = @ftell($this->stream);
         if ($result === false) {
             $error = error_get_last();
 
@@ -290,7 +291,7 @@ public function readImmediately(?int $max_bytes = null): string
             throw new Exception\RuntimeException('Pending operation.');
         }
 
-        if (null === $this->resource) {
+        if (!is_resource($this->stream)) {
             throw new Exception\AlreadyClosedException('Handle has already been closed.');
         }
 
@@ -303,7 +304,7 @@ public function readImmediately(?int $max_bytes = null): string
         }
 
         /** @psalm-suppress PossiblyInvalidArgument */
-        $result = $this->useSingleRead ? fread($this->resource, $max_bytes) : stream_get_contents($this->resource, $max_bytes);
+        $result = $this->useSingleRead ? fread($this->stream, $max_bytes) : stream_get_contents($this->stream, $max_bytes);
         if ($result === false) {
             /** @var array{message: string} $error */
             $error = error_get_last();
@@ -315,24 +316,25 @@ public function readImmediately(?int $max_bytes = null): string
     }
 
     /**
-     * @throws Exception\AlreadyClosedException If the handle has been already closed.
      * @throws Exception\RuntimeException If unable to close the handle.
      */
     public function close(): void
     {
-        if (null === $this->resource) {
-            throw new Exception\AlreadyClosedException('Handle has already been closed.');
-        }
-
-        /** @psalm-suppress PossiblyInvalidArgument */
-        $resource = $this->resource;
-        $this->resource = null;
-        $result = @fclose($resource);
-        if ($result === false) {
-            /** @var array{message: string} $error */
-            $error = error_get_last();
-
-            throw new Exception\RuntimeException($error['message'] ?? 'unknown error.');
+        if (is_resource($this->stream)) {
+            /** @psalm-suppress PossiblyInvalidArgument */
+            $stream = $this->stream;
+            $this->stream = null;
+            $result = @fclose($stream);
+            if ($result === false) {
+                /** @var array{message: string} $error */
+                $error = error_get_last();
+
+                throw new Exception\RuntimeException($error['message'] ?? 'unknown error.');
+            }
+        } else {
+            // Stream could be set to a non-null closed-resource,
+            // if manually closed using `fclose($handle->getStream)`.
+            $this->stream = null;
         }
 
         $this->readSuspension?->throw(throw new Exception\AlreadyClosedException('Handle has already been closed.'));
@@ -344,6 +346,11 @@ public function close(): void
      */
     public function getStream(): mixed
     {
-        return $this->resource;
+        return $this->stream;
+    }
+
+    public function __destruct()
+    {
+        $this->close();
     }
 }
diff --git a/src/Psl/TCP/Internal/Socket.php b/src/Psl/TCP/Internal/Socket.php
@@ -18,36 +18,36 @@
 final class Socket extends Internal\ResourceHandle implements TCP\SocketInterface
 {
     /**
-     * @param resource $resource
+     * @param resource $stream
      */
-    public function __construct($resource)
+    public function __construct($stream)
     {
-        parent::__construct($resource, read: true, write: true, seek: false);
+        parent::__construct($stream, read: true, write: true, seek: false);
     }
 
     /**
      * {@inheritDoc}
      */
     public function getLocalAddress(): Address
     {
-        if (null === $this->resource) {
+        if (null === $this->stream) {
             throw new Exception\AlreadyClosedException('Socket handle has already been closed.');
         }
 
         /** @psalm-suppress PossiblyInvalidArgument */
-        return Network\Internal\get_sock_name($this->resource);
+        return Network\Internal\get_sock_name($this->stream);
     }
 
     /**
      * {@inheritDoc}
      */
     public function getPeerAddress(): Address
     {
-        if (null === $this->resource) {
+        if (null === $this->stream) {
             throw new Exception\AlreadyClosedException('Socket handle has already been closed.');
         }
 
         /** @psalm-suppress PossiblyInvalidArgument */
-        return Network\Internal\get_peer_name($this->resource);
+        return Network\Internal\get_peer_name($this->stream);
     }
 }
diff --git a/src/Psl/Unix/Internal/Socket.php b/src/Psl/Unix/Internal/Socket.php
@@ -18,36 +18,36 @@
 final class Socket extends Internal\ResourceHandle implements Unix\SocketInterface
 {
     /**
-     * @param resource $resource
+     * @param resource $stream
      */
-    public function __construct($resource)
+    public function __construct($stream)
     {
-        parent::__construct($resource, read: true, write: true, seek: false);
+        parent::__construct($stream, read: true, write: true, seek: false);
     }
 
     /**
      * {@inheritDoc}
      */
     public function getLocalAddress(): Address
     {
-        if (null === $this->resource) {
+        if (null === $this->stream) {
             throw new Exception\AlreadyClosedException('Socket handle has already been closed.');
         }
 
         /** @psalm-suppress PossiblyInvalidArgument */
-        return Network\Internal\get_sock_name($this->resource);
+        return Network\Internal\get_sock_name($this->stream);
     }
 
     /**
      * {@inheritDoc}
      */
     public function getPeerAddress(): Address
     {
-        if (null === $this->resource) {
+        if (null === $this->stream) {
             throw new Exception\AlreadyClosedException('Socket handle has already been closed.');
         }
 
         /** @psalm-suppress PossiblyInvalidArgument */
-        return Network\Internal\get_peer_name($this->resource);
+        return Network\Internal\get_peer_name($this->stream);
     }
 }
diff --git a/tests/unit/File/ReadWriteHandleTest.php b/tests/unit/File/ReadWriteHandleTest.php
@@ -59,6 +59,21 @@ public function testReading(): void
         static::assertSame('derp', $file->read());
     }
 
+    public function testGetStream(): void
+    {
+        $file = File\temporary();
+        $file->writeAll('herpderp');
+
+        $file_stream = $file->getStream();
+        static::assertIsNotClosedResource($file_stream);
+
+        $file->close();
+
+        static::assertIsClosedResource($file_stream);
+
+        static::assertNull($file->getStream());
+    }
+
     public function testMustCreateExistingFile(): void
     {
         $this->expectException(InvariantViolationException::class);
@@ -155,10 +170,6 @@ public function provideOperations(): iterable
             static fn(File\ReadHandleInterface $handle) => $handle->readImmediately(),
         ];
 
-        yield [
-            static fn(File\HandleInterface $handle) => $handle->close(),
-        ];
-
         yield [
             static fn(File\HandleInterface $handle) => $handle->lock(File\LockType::EXCLUSIVE),
         ];
@@ -170,9 +181,5 @@ public function provideOperations(): iterable
         yield [
             static fn(File\HandleInterface $handle) => $handle->getSize(),
         ];
-
-        yield [
-            static fn(File\HandleInterface $handle) => $handle->close(),
-        ];
     }
 }