AK: Lower the requirements for InputStream::eof and rename it.

Consider the following snippet:

    void foo(InputStream& stream) {
        if(!stream.eof()) {
            u8 byte;
            stream >> byte;
        }
    }

There is a very subtle bug in this snippet, for some input streams eof()
might return false even if no more data can be read. In this case an
error flag would be set on the stream.

Until now I've always ensured that this is not the case, but this made
the implementation of eof() unnecessarily complicated.
InputFileStream::eof had to keep a ByteBuffer around just to make this
possible. That meant a ton of unnecessary copies just to get a reliable
eof().

In most cases it isn't actually necessary to have a reliable eof()
implementation.

In most other cases a reliable eof() is avaliable anyways because in
some cases like InputMemoryStream it is very easy to implement.

AK/BitStream.h

@@ -66,7 +66,7 @@ public:
         return true;
     }
 
-    bool eof() const override { return !m_next_byte.has_value() && m_stream.eof(); }
+    bool unreliable_eof() const override { return !m_next_byte.has_value() && m_stream.unreliable_eof(); }
 
     bool discard_or_error(size_t count) override
     {
@@ -86,6 +86,11 @@ public:
 
         size_t nread = 0;
         while (nread < count) {
+            if (m_stream.has_any_error()) {
+                set_fatal_error();
+                return 0;
+            }
+
             if (m_next_byte.has_value()) {
                 const auto bit = (m_next_byte.value() >> m_bit_offset) & 1;
                 result |= bit << nread;
@@ -93,9 +98,6 @@ public:
 
                 if (m_bit_offset++ == 7)
                     m_next_byte.clear();
-            } else if (m_stream.eof()) {
-                set_fatal_error();
-                return 0;
             } else {
                 m_stream >> m_next_byte;
                 m_bit_offset = 0;

AK/Buffered.h

@@ -77,7 +77,7 @@ public:
         return nread;
     }
 
-    virtual bool read_or_error(Bytes bytes) override
+    bool read_or_error(Bytes bytes) override
     {
         if (read(bytes) < bytes.size()) {
             set_fatal_error();
@@ -87,7 +87,9 @@ public:
         return true;
     }
 
-    virtual bool eof() const
+    bool unreliable_eof() const override { return m_buffer_remaining == 0 && m_stream.unreliable_eof(); }
+
+    bool eof() const
     {
         if (m_buffer_remaining > 0)
             return false;
@@ -97,7 +99,7 @@ public:
         return m_buffer_remaining == 0;
     }
 
-    virtual bool discard_or_error(size_t count) override
+    bool discard_or_error(size_t count) override
     {
         size_t ndiscarded = 0;
         while (ndiscarded < count) {

AK/CircularDuplexStream.h

@@ -112,7 +112,8 @@ public:
         return true;
     }
 
-    bool eof() const override { return m_queue.size() == 0; }
+    bool unreliable_eof() const override { return eof(); }
+    bool eof() const { return m_queue.size() == 0; }
 
     size_t remaining_contigous_space() const
     {

AK/MemoryStream.h

@@ -40,7 +40,8 @@ public:
     {
     }
 
-    bool eof() const override { return m_offset >= m_bytes.size(); }
+    bool unreliable_eof() const override { return eof(); }
+    bool eof() const { return m_offset >= m_bytes.size(); }
 
     size_t read(Bytes bytes) override
     {
@@ -167,7 +168,8 @@ class DuplexMemoryStream final : public DuplexStream {
 public:
     static constexpr size_t chunk_size = 4 * 1024;
 
-    bool eof() const override { return m_write_offset == m_read_offset; }
+    bool unreliable_eof() const override { return eof(); }
+    bool eof() const { return m_write_offset == m_read_offset; }
 
     bool discard_or_error(size_t count) override
     {

AK/Stream.h

@@ -75,10 +75,22 @@ namespace AK {
 
 class InputStream : public virtual AK::Detail::Stream {
 public:
-    // Does nothing and returns zero if there is already an error.
+    // Reads at least one byte unless none are requested or none are avaliable. Does nothing
+    // and returns zero if there is already an error.
     virtual size_t read(Bytes) = 0;
+
+    // If this function returns true, then no more data can be read. If read(Bytes) previously
+    // returned zero even though bytes were requested, then the inverse is true as well.
+    virtual bool unreliable_eof() const = 0;
+
+    // Some streams additionally define a method with the signature:
+    //
+    //     bool eof() const;
+    //
+    // This method has the same semantics as unreliable_eof() but returns true if and only if no
+    // more data can be read. (A failed read is not necessary.)
+
     virtual bool read_or_error(Bytes) = 0;
-    virtual bool eof() const = 0;
     virtual bool discard_or_error(size_t count) = 0;
 };
 

AK/String.h

@@ -285,16 +285,15 @@ inline InputStream& operator>>(InputStream& stream, String& string)
     StringBuilder builder;
 
     for (;;) {
-        if (stream.eof()) {
-            string = nullptr;
+        char next_char;
+        stream >> next_char;
 
+        if (stream.has_any_error()) {
             stream.set_fatal_error();
+            string = nullptr;
             return stream;
         }
 
-        char next_char;
-        stream >> next_char;
-
         if (next_char) {
             builder.append(next_char);
         } else {

Libraries/LibCompress/Deflate.cpp

@@ -290,7 +290,7 @@ bool DeflateDecompressor::discard_or_error(size_t count)
 
     size_t ndiscarded = 0;
     while (ndiscarded < count) {
-        if (eof()) {
+        if (unreliable_eof()) {
             set_fatal_error();
             return false;
         }
@@ -301,7 +301,7 @@ bool DeflateDecompressor::discard_or_error(size_t count)
     return true;
 }
 
-bool DeflateDecompressor::eof() const { return m_state == State::Idle && m_read_final_bock; }
+bool DeflateDecompressor::unreliable_eof() const { return m_state == State::Idle && m_read_final_bock; }
 
 Optional<ByteBuffer> DeflateDecompressor::decompress_all(ReadonlyBytes bytes)
 {
@@ -310,7 +310,7 @@ Optional<ByteBuffer> DeflateDecompressor::decompress_all(ReadonlyBytes bytes)
     OutputMemoryStream output_stream;
 
     u8 buffer[4096];
-    while (!deflate_stream.has_any_error() && !deflate_stream.eof()) {
+    while (!deflate_stream.has_any_error() && !deflate_stream.unreliable_eof()) {
         const auto nread = deflate_stream.read({ buffer, sizeof(buffer) });
         output_stream.write_or_error({ buffer, nread });
     }

Libraries/LibCompress/Deflate.h

@@ -92,7 +92,8 @@ public:
     size_t read(Bytes) override;
     bool read_or_error(Bytes) override;
     bool discard_or_error(size_t) override;
-    bool eof() const override;
+
+    bool unreliable_eof() const override;
 
     static Optional<ByteBuffer> decompress_all(ReadonlyBytes);
 

Libraries/LibCompress/Gzip.cpp

@@ -68,7 +68,7 @@ GzipDecompressor::~GzipDecompressor()
 // FIXME: Again, there are surely a ton of bugs because the code doesn't check for read errors.
 size_t GzipDecompressor::read(Bytes bytes)
 {
-    if (has_any_error())
+    if (has_any_error() || m_eof)
         return 0;
 
     if (m_current_member.has_value()) {
@@ -99,12 +99,14 @@ size_t GzipDecompressor::read(Bytes bytes)
 
         return nread;
     } else {
-        if (m_input_stream.eof())
-            return 0;
-
         BlockHeader header;
         m_input_stream >> Bytes { &header, sizeof(header) };
 
+        if (m_input_stream.handle_any_error()) {
+            m_eof = true;
+            return 0;
+        }
+
         if (!header.valid_magic_number() || !header.supported_by_implementation()) {
             set_fatal_error();
             return 0;
@@ -147,7 +149,7 @@ bool GzipDecompressor::discard_or_error(size_t count)
 
     size_t ndiscarded = 0;
     while (ndiscarded < count) {
-        if (eof()) {
+        if (unreliable_eof()) {
             set_fatal_error();
             return false;
         }
@@ -165,7 +167,7 @@ Optional<ByteBuffer> GzipDecompressor::decompress_all(ReadonlyBytes bytes)
     OutputMemoryStream output_stream;
 
     u8 buffer[4096];
-    while (!gzip_stream.has_any_error() && !gzip_stream.eof()) {
+    while (!gzip_stream.has_any_error() && !gzip_stream.unreliable_eof()) {
         const auto nread = gzip_stream.read({ buffer, sizeof(buffer) });
         output_stream.write_or_error({ buffer, nread });
     }
@@ -176,15 +178,6 @@ Optional<ByteBuffer> GzipDecompressor::decompress_all(ReadonlyBytes bytes)
     return output_stream.copy_into_contiguous_buffer();
 }
 
-bool GzipDecompressor::eof() const
-{
-    if (m_current_member.has_value()) {
-        // FIXME: There is an ugly edge case where we read the whole deflate block
-        //        but haven't read CRC32 and ISIZE.
-        return current_member().m_stream.eof() && m_input_stream.eof();
-    } else {
-        return m_input_stream.eof();
-    }
-}
+bool GzipDecompressor::unreliable_eof() const { return m_eof; }
 
 }

Libraries/LibCompress/Gzip.h

@@ -39,7 +39,8 @@ public:
     size_t read(Bytes) override;
     bool read_or_error(Bytes) override;
     bool discard_or_error(size_t) override;
-    bool eof() const override;
+
+    bool unreliable_eof() const override;
 
     static Optional<ByteBuffer> decompress_all(ReadonlyBytes);
 
@@ -87,6 +88,8 @@ private:
 
     InputStream& m_input_stream;
     Optional<Member> m_current_member;
+
+    bool m_eof { false };
 };
 
 }

Libraries/LibCore/FileStream.h

@@ -68,25 +68,8 @@ public:
         if (has_any_error())
             return 0;
 
-        auto nread = m_buffered.bytes().copy_trimmed_to(bytes);
-
-        m_buffered.bytes().slice(nread, m_buffered.size() - nread).copy_to(m_buffered);
-        m_buffered.trim(m_buffered.size() - nread);
-
-        while (nread < bytes.size()) {
-            if (m_file->eof())
-                return nread;
-
-            if (m_file->has_error()) {
-                set_fatal_error();
-                return 0;
-            }
-
-            const auto buffer = m_file->read(bytes.size() - nread);
-            nread += buffer.bytes().copy_to(bytes.slice(nread));
-        }
-
-        return nread;
+        const auto buffer = m_file->read(bytes.size());
+        return buffer.bytes().copy_to(bytes);
     }
 
     bool read_or_error(Bytes bytes) override
@@ -99,34 +82,9 @@ public:
         return true;
     }
 
-    bool discard_or_error(size_t count) override
-    {
-        u8 buffer[4096];
-
-        size_t ndiscarded = 0;
-        while (ndiscarded < count && !eof())
-            ndiscarded += read({ buffer, min<size_t>(count - ndiscarded, sizeof(buffer)) });
-
-        if (eof()) {
-            set_fatal_error();
-            return false;
-        }
-
-        return true;
-    }
-
-    bool eof() const override
-    {
-        if (m_buffered.size() > 0)
-            return false;
-
-        if (m_file->eof())
-            return true;
+    bool discard_or_error(size_t count) override { return m_file->seek(count, IODevice::SeekMode::FromCurrentPosition); }
 
-        m_buffered = m_file->read(4096);
-
-        return m_buffered.size() == 0;
-    }
+    bool unreliable_eof() const override { return m_file->eof(); }
 
     void close()
     {
@@ -137,8 +95,7 @@ public:
 private:
     InputFileStream() = default;
 
-    mutable NonnullRefPtr<File> m_file;
-    mutable ByteBuffer m_buffered;
+    NonnullRefPtr<File> m_file;
 };
 
 class OutputFileStream : public OutputStream {

Userland/gunzip.cpp

@@ -34,7 +34,7 @@ static void decompress_file(Buffered<Core::InputFileStream>& input_stream, Buffe
 
     u8 buffer[4096];
 
-    while (!gzip_stream.eof()) {
+    while (!gzip_stream.unreliable_eof()) {
         const auto nread = gzip_stream.read({ buffer, sizeof(buffer) });
         output_stream.write_or_error({ buffer, nread });
     }