Complete implementing genuine file descriptors

Author: BCSharp

Src/IronPython.Modules/nt.cs

@@ -351,19 +351,24 @@ public static int dup(CodeContext/*!*/ context, int fd) {
             PythonFileManager fileManager = context.LanguageContext.FileManager;
 
             StreamBox streams = fileManager.GetStreams(fd); // OSError if fd not valid
-            fileManager.EnsureRefStreams(streams);
-            fileManager.AddRefStreams(streams);
             if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) {
+                fileManager.EnsureRefStreams(streams);
+                fileManager.AddRefStreams(streams);
                 return fileManager.Add(new(streams));
             } else {
-                return fileManager.Add(UnixDup(fd), new(streams));
-            }
-
-            // Isolate Mono.Unix from the rest of the method so that we don't try to load the Mono.Unix assembly on Windows.
-            static int UnixDup(int fd) {
-                int res = Mono.Unix.Native.Syscall.dup(fd);
-                if (res < 0) throw GetLastUnixError();
-                return res;
+                if (!streams.IsSingleStream && fd is 1 or 2) {
+                    // If there is a separate write stream, dupping over stout or sderr uses write stream's file descriptor
+                    fd = streams.WriteStream is FileStream fs ? fs.SafeFileHandle.DangerousGetHandle().ToInt32() : fd;
+                }
+                int fd2 = UnixDup(fd, -1, out Stream? dupstream);
+                if (dupstream is not null) {
+                    return fileManager.Add(fd2, new(dupstream));
+                } else {
+                    // Share the same set of streams between the original and the dupped descriptor
+                    fileManager.EnsureRefStreams(streams);
+                    fileManager.AddRefStreams(streams);
+                    return fileManager.Add(fd2, new(streams));
+                }
             }
         }
 
@@ -384,22 +389,45 @@ public static int dup2(CodeContext/*!*/ context, int fd, int fd2) {
                 close(context, fd2);
             }
 
-            // TODO: race condition: `open` or `dup` on another thread may occupy fd2 
+            // TODO: race condition: `open` or `dup` on another thread may occupy fd2 (simulated desctiptors only)
 
-            fileManager.EnsureRefStreams(streams);
-            fileManager.AddRefStreams(streams);
             if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) {
+                fileManager.EnsureRefStreams(streams);
+                fileManager.AddRefStreams(streams);
                 return fileManager.Add(fd2, new(streams));
             } else {
-                return fileManager.Add(UnixDup2(fd, fd2), new(streams));
+                if (!streams.IsSingleStream && fd is 1 or 2) {
+                    // If there is a separate write stream, dupping over stout or sderr uses write stream's file descriptor
+                    fd = streams.WriteStream is FileStream fs ? fs.SafeFileHandle.DangerousGetHandle().ToInt32() : fd;
+                }
+                fd2 = UnixDup(fd, fd2, out Stream? dupstream); // closes fd2 atomically if reopened in the meantime
+                fileManager.Remove(fd2);
+                if (dupstream is not null) {
+                    return fileManager.Add(fd2, new(dupstream));
+                } else {
+                    // Share the same set of streams between the original and the dupped descriptor
+                    fileManager.EnsureRefStreams(streams);
+                    fileManager.AddRefStreams(streams);
+                    return fileManager.Add(fd2, new(streams));
+                }
             }
+        }
 
-            // Isolate Mono.Unix from the rest of the method so that we don't try to load the Mono.Unix assembly on Windows.
-            static int UnixDup2(int fd, int fd2) {
-                int res = Mono.Unix.Native.Syscall.dup2(fd, fd2);
-                if (res < 0) throw GetLastUnixError();
-                return res;
+
+        private static int UnixDup(int fd, int fd2, out Stream? stream) {
+            int res = fd2 < 0 ? Mono.Unix.Native.Syscall.dup(fd) : Mono.Unix.Native.Syscall.dup2(fd, fd2);
+            if (res < 0) throw GetLastUnixError();
+            if (ClrModule.IsMono) {
+                // This does not work on .NET, probably because .NET FileStream is not aware of Mono.Unix.UnxiStream
+                stream = new Mono.Unix.UnixStream(res, ownsHandle: true);
+            } else {
+                // This does not work 100% correctly on .NET, probably because each FileStream has its own read/write cursor
+                // (it should be shared between dupped descriptors)
+                //stream = new FileStream(new SafeFileHandle((IntPtr)res, ownsHandle: true), FileAccess.ReadWrite);
+                // Accidentaly, this would also not work on Mono: https://github.com/mono/mono/issues/12783
+                stream = null; // Handle stream sharing in PythonFileManager
             }
+            return res;
         }
 
 #if FEATURE_PROCESS
@@ -849,25 +877,27 @@ public static object open(CodeContext/*!*/ context, [NotNone] string path, int f
                 FileMode fileMode = FileModeFromFlags(flags);
                 FileAccess access = FileAccessFromFlags(flags);
                 FileOptions options = FileOptionsFromFlags(flags);
-                Stream fs;
+                Stream s;
+                FileStream? fs;
                 if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && IsNulFile(path)) {
-                    fs = Stream.Null;
+                    fs = null;
+                    s = Stream.Null;
                 } else if (access == FileAccess.Read && (fileMode == FileMode.CreateNew || fileMode == FileMode.Create || fileMode == FileMode.Append)) {
                     // .NET doesn't allow Create/CreateNew w/ access == Read, so create the file, then close it, then
                     // open it again w/ just read access.
                     fs = new FileStream(path, fileMode, FileAccess.Write, FileShare.None);
                     fs.Close();
-                    fs = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.ReadWrite, DefaultBufferSize, options);
+                    s = fs = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.ReadWrite, DefaultBufferSize, options);
                 } else if (access == FileAccess.ReadWrite && fileMode == FileMode.Append) {
-                    fs = new FileStream(path, FileMode.Append, FileAccess.Write, FileShare.ReadWrite, DefaultBufferSize, options);
+                    s = fs = new FileStream(path, FileMode.Append, FileAccess.Write, FileShare.ReadWrite, DefaultBufferSize, options);
                 } else {
-                    fs = new FileStream(path, fileMode, access, FileShare.ReadWrite, DefaultBufferSize, options);
+                    s = fs = new FileStream(path, fileMode, access, FileShare.ReadWrite, DefaultBufferSize, options);
                 }
 
                 if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) {
-                    return context.LanguageContext.FileManager.Add(new(fs));
+                    return context.LanguageContext.FileManager.Add(new(s));
                 } else {
-                    return context.LanguageContext.FileManager.Add((int)fs.SafeFileHandle.DangerousGetHandle(), new(fs));
+                    return context.LanguageContext.FileManager.Add((int)fs!.SafeFileHandle.DangerousGetHandle(), new(s));
                 }
             } catch (Exception e) {
                 throw ToPythonException(e, path);
@@ -916,14 +946,14 @@ public static PythonTuple pipe(CodeContext context) {
             } else {
                 var pipeStreams = CreatePipeStreamsUnix();
                 return PythonTuple.MakeTuple(
-                    manager.Add((int)pipeStreams.Item1.SafeFileHandle.DangerousGetHandle(), new(pipeStreams.Item1)),
-                    manager.Add((int)pipeStreams.Item2.SafeFileHandle.DangerousGetHandle(), new(pipeStreams.Item2))
+                    manager.Add(pipeStreams.Item1, new(pipeStreams.Item2)),
+                    manager.Add(pipeStreams.Item3, new(pipeStreams.Item4))
                 );
             }
 
-            static Tuple<Stream, Stream> CreatePipeStreamsUnix() {
+            static Tuple<int, Stream, int, Stream> CreatePipeStreamsUnix() {
                 Mono.Unix.UnixPipes pipes = Mono.Unix.UnixPipes.CreatePipes();
-                return Tuple.Create<Stream, Stream>(pipes.Reading, pipes.Writing);
+                return Tuple.Create<int, Stream, int, Stream>(pipes.Reading.Handle, pipes.Reading, pipes.Writing.Handle, pipes.Writing);
             }
         }
 #endif

Src/IronPython/Modules/_fileio.cs

@@ -239,7 +239,7 @@ static Exception BadMode(string mode) {
 
             [Documentation("close() -> None.  Close the file.\n\n"
                 + "A closed file cannot be used for further I/O operations.  close() may be"
-                + "called more than once without error.  Changes the fileno to -1."
+                + "called more than once without error."
                 )]
             public override void close(CodeContext/*!*/ context) {
                 if (_closed) {

Src/IronPython/Runtime/PythonFileManager.cs

@@ -10,6 +10,7 @@
 using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
 using System.IO;
+using System.Runtime.InteropServices;
 using System.Runtime.Versioning;
 
 using Microsoft.Scripting.Runtime;
@@ -87,13 +88,16 @@ public bool IsConsoleStream() {
 
             // Isolate Mono.Unix from the rest of the method so that we don't try to load the Mono.Unix assembly on Windows.
             bool isattyUnix() {
-                // TODO: console streams may be dupped to differend FD numbers, do not use hard-coded 0, 1, 2
-                return StreamType switch {
-                    ConsoleStreamType.Input => Mono.Unix.Native.Syscall.isatty(0),
-                    ConsoleStreamType.Output => Mono.Unix.Native.Syscall.isatty(1),
-                    ConsoleStreamType.ErrorOutput => Mono.Unix.Native.Syscall.isatty(2),
-                    _ => false
-                };
+                if (Id >= 0) {
+                    return Mono.Unix.Native.Syscall.isatty(Id);
+                } else {
+                    return StreamType switch {
+                        ConsoleStreamType.Input => Mono.Unix.Native.Syscall.isatty(0),
+                        ConsoleStreamType.Output => Mono.Unix.Native.Syscall.isatty(1),
+                        ConsoleStreamType.ErrorOutput => Mono.Unix.Native.Syscall.isatty(2),
+                        _ => false
+                    };
+                }
             }
         }
 
@@ -213,7 +217,8 @@ internal class PythonFileManager {
         private int _current = _offset; // lowest potentially unused key in _objects at or above _offset
         private readonly ConcurrentDictionary<Stream, int> _refs = new();
 
-        // Mandatory Add for Unix, on Windows only for dup2 case
+        // This version of Add is used with genuine file descriptors (Unix).
+        // Exception: support dup2 on all frameworks/platfroms.
         public int Add(int id, StreamBox streams) {
             ContractUtils.RequiresNotNull(streams, nameof(streams));
             ContractUtils.Requires(streams.Id < 0, nameof(streams));
@@ -228,6 +233,8 @@ public int Add(int id, StreamBox streams) {
             }
         }
 
+        // This version of Add is used for emulated file descriptors.
+        // Must not be used on Unix.
         [SupportedOSPlatform("windows")]
         public int Add(StreamBox streams) {
             ContractUtils.RequiresNotNull(streams, nameof(streams));
@@ -280,7 +287,13 @@ public int GetOrAssignId(StreamBox streams) {
             lock (_synchObject) {
                 int res = streams.Id;
                 if (res == -1) {
-                    res = Add(streams);
+                    if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) {
+                        res = Add(streams);
+                    } else {
+                        res = GetGenuineFileDescriptor(streams.ReadStream);
+                        if (res < 0) throw new InvalidOperationException("stream not associated with a file descriptor");
+                        Add(res, streams);
+                    }
                 }
                 return res;
             }
@@ -313,5 +326,17 @@ public bool DerefStreamsAndCloseIfLast(StreamBox streams) {
         public bool ValidateFdRange(int fd) {
             return fd >= 0 && fd < LIMIT_OFILE;
         }
+
+        [UnsupportedOSPlatform("windows")]
+        private static int GetGenuineFileDescriptor(Stream stream) {
+            return stream switch {
+                FileStream fs => checked((int)fs.SafeFileHandle.DangerousGetHandle()),
+#if FEATURE_PIPES
+                System.IO.Pipes.PipeStream ps => checked((int)ps.SafePipeHandle.DangerousGetHandle()),
+#endif
+                Mono.Unix.UnixStream us => us.Handle,
+                _ => -1
+            };
+        }
     }
 }

Tests/modules/io_related/test_fd.py

@@ -9,7 +9,7 @@
 import sys
 import unittest
 
-from iptest import IronPythonTestCase, is_cli, run_test
+from iptest import IronPythonTestCase, is_cli, is_posix, run_test
 from threading import Timer
 
 flags = os.O_CREAT | os.O_TRUNC | os.O_RDWR
@@ -45,12 +45,11 @@ def test_dup2(self):
 
         # Assigning to self must be a no-op.
         self.assertEqual(os.dup2(fd, fd), fd if is_cli or sys.version_info >= (3,7) else None)
-        self.assertTrue(is_open(fd))
 
         # The source must be valid.
-        self.assertRaisesMessage(OSError, "[Errno 9] Bad file descriptor", os.dup2, -1, fd)
+        self.assertRaisesMessage(OSError, "[Errno 9] Bad file descriptor" if is_cli or sys.version_info >= (3,5) else "[Errno 0] Error", os.dup2, -1, fd)
         self.assertTrue(is_open(fd))
-        self.assertRaisesMessage(OSError, "[Errno 9] Bad file descriptor", os.dup2, 99, fd)
+        self.assertRaisesMessage(OSError, "[Errno 9] Bad file descriptor", os.dup2, fd + 10000, fd)
         self.assertTrue(is_open(fd))
 
         # If the source is not open, then the destination is unaffected.
@@ -67,16 +66,16 @@ def test_dup2(self):
         # Using dup2 can skip fds.
         self.assertEqual(os.dup2(fd, fd + 2), fd + 2 if is_cli or sys.version_info >= (3,7) else None)
         self.assertTrue(is_open(fd))
-        self.assertTrue(not is_open(fd + 1))
+        self.assertFalse(is_open(fd + 1))
         self.assertTrue(is_open(fd + 2))
 
         # Verify that dup2 closes the previous occupant of a fd.
-        self.assertEqual(os.open(os.devnull, os.O_RDWR, 0o600), fd + 1)
-        self.assertEqual(os.dup2(fd + 1, fd), fd if is_cli or sys.version_info >= (3,7) else None)
+        fdn = os.open(os.devnull, os.O_RDWR, 0o600)
+        self.assertEqual(os.dup2(fdn, fd), fd if is_cli or sys.version_info >= (3,7) else None)
 
         self.assertTrue(is_open_nul(fd))
-        self.assertTrue(is_open_nul(fd + 1))
-        os.close(fd + 1)
+        self.assertTrue(is_open_nul(fdn))
+        os.close(fdn)
         self.assertTrue(is_open_nul(fd))
         self.assertEqual(os.write(fd, b"1"), 1)
 
@@ -87,7 +86,7 @@ def test_dup2(self):
         self.assertEqual(os.read(fd, 1), b"2")
 
         os.close(fd)
-        # fd+1 is already closed
+        # fdn is already closed
         os.close(fd + 2)
         os.unlink(test_filename)
 
@@ -101,29 +100,33 @@ def test_dup(self):
 
         # The source must be valid.
         self.assertRaisesMessage(OSError, "[Errno 9] Bad file descriptor", os.dup, -1)
-        self.assertRaisesMessage(OSError, "[Errno 9] Bad file descriptor", os.dup, 99)
+        self.assertRaisesMessage(OSError, "[Errno 9] Bad file descriptor", os.dup, fd1 + 10000)
 
         # Test basic functionality.
         fd2 = os.dup(fd1)
-        self.assertTrue(fd2 == fd1 + 1)
+        if not (is_cli and is_posix):
+            # On IronPython/Posix, the first call to dup or dup2 may load Mono.Unix.dll and the corresponding `.so`
+            # This makes the fd numbers less predictable
+            self.assertEqual(fd2, fd1 + 1)
         self.assertTrue(is_open(fd2))
         self.assertTrue(is_open(fd1))
         os.close(fd1)
-        self.assertTrue(not is_open(fd1))
+        self.assertFalse(is_open(fd1))
         self.assertTrue(is_open(fd2))
 
-        # dup uses the lowest-numbered unused descriptor for the new descriptor.
         fd3 = os.dup(fd2)
-        self.assertEqual(fd3, fd1)
+        # dup uses the lowest-numbered unused descriptor for the new descriptor.
+        if not (is_cli and is_posix):
+            self.assertEqual(fd3, fd1)
 
-        # writing though the duplicated fd writes to the same file
-        self.assertEqual(os.write(fd2, b"fd2"), 3)
-        self.assertEqual(os.write(fd3, b"fd3"), 3)
-        self.assertEqual(os.write(fd2, b"fd2-again"), 9)
+        # writing through the duplicated fd writes to the same file
+        self.assertEqual(os.write(fd2, b"(fd2)"), 5)
+        self.assertEqual(os.write(fd3, b"(=====fd3=====)"), 15)
+        self.assertEqual(os.write(fd2, b"(fd2-again)"), 11)
         os.close(fd3)
 
         self.assertEqual(os.lseek(fd2, 0, os.SEEK_SET), 0)
-        self.assertEqual(os.read(fd2, 15), b"fd2fd3fd2-again")
+        self.assertEqual(os.read(fd2, 5 + 15 + 11), b"(fd2)(=====fd3=====)(fd2-again)")
 
         # cleanup
         os.close(fd2)
@@ -133,20 +136,20 @@ def test_dup_file(self):
         test_filename = "tmp_%d.dup-file.test" % os.getpid()
 
         file1 = open(test_filename, 'w+')
-        file1.write("file1")
+        file1.write("(file1)")
         file1.flush()
 
         fd2 = os.dup(file1.fileno())
         file2 = open(fd2, 'w+')
         self.assertNotEqual(file1.fileno(), file2.fileno())
 
-        file2.write("file2")
+        file2.write("(======file2======)")
         file2.flush()
-        file1.write("file1-again")
+        file1.write("(file1-again)")
         file1.close()
 
         file2.seek(0)
-        self.assertEqual(file2.read(), "file1file2file1-again")
+        self.assertEqual(file2.read(), "(file1)(======file2======)(file1-again)")
 
         file2.close()
         os.unlink(test_filename)

Tests/test_file.py

@@ -787,6 +787,10 @@ def test_open_abplus(self):
         with open(self.temp_file, "ab+") as f:
             f.write(b"abc")
             f.seek(0)
-            self.assertEqual(f.read(), b"abc")
+            self.assertEqual(f.read(2), b"ab")
+            f.write(b"def")
+            self.assertEqual(f.read(2), b"")
+            f.seek(0)
+            self.assertEqual(f.read(6), b"abcdef")
 
 run_test(__name__)