Fix Issue 24165 - Failed readf leaves File in inconsistent state

Previously, a failed call to readf resulted in multiple copies of the
same LockingTextReader being destroyed. Since LockingTextReader's
destructor calls ungetc on the last-read character, this caused that
character to appear multiple times in subsequent reads from the File.

This change ensures that the destructor in question is only run once by
making LockingTextReader a reference-counted type.

Ideally, to avoid unnecessary overhead, this issue would have been fixed
by making LockingTextReader non-copyable. However, non-copyable ranges
are poorly-supported by Phobos, and this approach would have required
extensive changes to several other modules, including changes to the
interfaces of some public symbols.

Author: pbackus

std/stdio.d

@@ -3993,79 +3993,103 @@ enum LockType
 
 struct LockingTextReader
 {
-    private File _f;
-    private char _front;
-    private bool _hasChar;
-
-    this(File f)
+    private static struct Impl
     {
-        import std.exception : enforce;
-        enforce(f.isOpen, "LockingTextReader: File must be open");
-        _f = f;
-        _FLOCK(_f._p.handle);
-    }
+        private File _f;
+        private char _front;
+        private bool _hasChar;
 
-    this(this)
-    {
-        _FLOCK(_f._p.handle);
-    }
+        this(File f)
+        {
+            import std.exception : enforce;
+            enforce(f.isOpen, "LockingTextReader: File must be open");
+            _f = f;
+            _FLOCK(_f._p.handle);
+        }
 
-    ~this()
-    {
-        if (_hasChar)
-            ungetc(_front, cast(FILE*)_f._p.handle);
+        @disable this(this);
 
-        // File locking has its own reference count
-        if (_f.isOpen) _FUNLOCK(_f._p.handle);
-    }
+        ~this()
+        {
+            if (_hasChar)
+                ungetc(_front, cast(FILE*)_f._p.handle);
 
-    void opAssign(LockingTextReader r)
-    {
-        import std.algorithm.mutation : swap;
-        swap(this, r);
-    }
+            // File locking has its own reference count
+            if (_f.isOpen) _FUNLOCK(_f._p.handle);
+        }
 
-    @property bool empty()
-    {
-        if (!_hasChar)
+        void opAssign(typeof(this) r)
+        {
+            import std.algorithm.mutation : swap;
+            swap(this, r);
+        }
+
+        @property bool empty()
         {
-            if (!_f.isOpen || _f.eof)
-                return true;
-            immutable int c = _FGETC(cast(_iobuf*) _f._p.handle);
-            if (c == EOF)
+            if (!_hasChar)
             {
-                .destroy(_f);
-                return true;
+                if (!_f.isOpen || _f.eof)
+                    return true;
+                immutable int c = _FGETC(cast(_iobuf*) _f._p.handle);
+                if (c == EOF)
+                {
+                    .destroy(_f);
+                    return true;
+                }
+                _front = cast(char) c;
+                _hasChar = true;
             }
-            _front = cast(char) c;
-            _hasChar = true;
+            return false;
         }
-        return false;
-    }
 
-    @property char front()
-    {
-        if (!_hasChar)
+        @property char front()
         {
-            version (assert)
+            if (!_hasChar)
             {
-                import core.exception : RangeError;
-                if (empty)
-                    throw new RangeError();
+                version (assert)
+                {
+                    import core.exception : RangeError;
+                    if (empty)
+                        throw new RangeError();
+                }
+                else
+                {
+                    empty;
+                }
             }
-            else
-            {
+            return _front;
+        }
+
+        void popFront()
+        {
+            if (!_hasChar)
                 empty;
-            }
+            _hasChar = false;
         }
-        return _front;
+    }
+
+    import std.typecons : SafeRefCounted, borrow;
+
+    private SafeRefCounted!Impl impl;
+
+    this(File f)
+    {
+        impl = SafeRefCounted!Impl(f);
+    }
+
+    @property bool empty()
+    {
+        return impl.borrow!((ref r) => r.empty);
+    }
+
+    @property char front()
+    {
+        return impl.borrow!((ref r) => r.front);
     }
 
     void popFront()
     {
-        if (!_hasChar)
-            empty;
-        _hasChar = false;
+        impl.borrow!((ref r) { r.popFront; });
     }
 }
 
@@ -4143,6 +4167,29 @@ struct LockingTextReader
         fr.readf("%s;%s;%s;%s\n", &nom, &fam, &nam, &ot);
 }
 
+// https://issues.dlang.org/show_bug.cgi?id=24165
+@system unittest
+{
+    // @system due to readf
+   static import std.file;
+   import std.algorithm.iteration : joiner, map;
+   import std.algorithm.searching : canFind;
+   import std.array : array;
+   import std.utf : byChar;
+
+   string content = "-hello";
+   auto deleteme = testFilename();
+   std.file.write(deleteme, content);
+   scope(exit) std.file.remove(deleteme);
+   File f = File(deleteme);
+   int n;
+   try f.readf("%d", n);
+   catch (Exception e) {}
+   // Data read must match what was written
+   char[] result = f.byLine(Yes.keepTerminator).map!byChar.joiner.array;
+   assert(content.canFind(result));
+}
+
 /**
  * Indicates whether `T` is a file handle, i.e. the type
  * is implicitly convertable to $(LREF File) or a pointer to a