Use genuine file descriptors on Posix #1832

BCSharp · 2024-12-07T05:08:57Z

Up until now, file descriptors provided by the Python API (e.g f.fileno()) were emulated, that is, they were "fake". This was necessary on Windows, since .NET does not run on top of the C library, but on POSIX systems, at the low level, the genuine file descriptors are used. This PR exposes those genuine file descriptors on the Python API level, making path for a set of POSIX-specific modules, like fcntl and termios.

I thought that Mono would give me the most problems, because of the limitation of the FileStream constructor, but in the end, it was .NET: it does not share the read/write pointer with duplicated descriptors. I have implemented a workaround (details in the new developer's documentation file), but I am not happy with it. The important thing is that there are no regressions that I know of, and the usage scenarios I have seen in Python various modules should be working.

Here is the overview of the remaining open points:

On .NET, FileStream is shared between duplicated descriptors, like on Windows. This is a workaround. There are some scenarios for which it doesn't work correctly (see documentation).
On Mono, after file descriptor duplication, UnixStream is used iso FileStream. I do not know if there are any unexpected side effect (I expect none).
Handling cases with separate read and write streams is tricky. Currently, it is being used for file open mode 'ab+'. The code includes a workaround for copying over standard descriptors, which is the most common case. Luckily, I think that the separate streams mechanism can be abolished altogether. I plan it for a separate PR.
There is still the race condition left in dup2. It was there since forever, so it is not a regression. Luckily, it happens only under a very uncommon condition.

slozier

I know it's still a draft, just a few quick comments.

slozier · 2024-12-08T03:13:00Z

Documentation/file-descriptors.md

+
+In this way, the file descriptor on the `PythonFileManager` level is the same as the file descriptor used by `FileStream`.
+
+Unfortunately, on .NET, somehow, the two independent read/write positions. This is not how duplicated file descriptors should work: both descriptors should point to the same file description structure and share the read/seek/write position. In practice, on .NET, writing through the second file object will overwrite data already written through the first file object. In regular Unix applications (incl. CPython), the subsequent writes append data, regardless which file object is used. The same principle should apply to reads.


Missing a word in first sentence.

slozier · 2024-12-08T03:24:48Z

Src/IronPython.Modules/nt.cs

-            fileManager.EnsureRefStreams(streams);
-            fileManager.AddRefStreams(streams);
-            return fileManager.Add(new(streams));
+            if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) {


I'm thinking the platform agnostic version should be the else clause. Though it's possible we haven't been very consistent in doing this. For example, something running on Blazor would fall into the else clause (although the nt module might not be super relevant in that case).

By "the platform agnostic version" do you mean the emulated descriptors version? And that the genuine file descriptors should be limited to Linux and maxOS only? In the past, the platform specific conditions and branches were dictated by the available guard SupportedOSPlatform, but now that we have also UnsupportedOSPlatform anything is possible. I thought that UnsupportedOSPlatform was merely a convenience, but if we were to be considerate of any other platforms than the ones we officially support, it becomes a necessity. OTOH, I see that IsOSPlatform accepts only 4 enum values (the 4th being FreeBSD) so how it is supposed to work on Blazor?

Yes, I meant using the emulated version when the platform is unknown.

OSPlatform can be constructed using arbitrary strings. On .NET it just dispatches to System.OperatingSystem which contains all sorts of new goodies. For Blazor the platform string is BROWSER. But there are a bunch of others beyond what's in RuntimeInformation.

slozier · 2024-12-08T03:29:07Z

Src/IronPython.Modules/nt.cs

+            } else {
+                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;


Is it documented somewhere that this is the file descriptor? Or would we be relying on an implementation detail? Not that I'm against it, just curious...

This is an implementation detail. The documentation only reads that it is "platform-specific", as far as I could find.

BTW, this piece of code is a hack around the dual-stream case (used for mode ab+), which I intend to abolish, in probably the next PR, so this piece will be deleted as well.

slozier · 2024-12-08T03:29:29Z

Src/IronPython.Modules/nt.cs

@@ -373,11 +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)


Typo: desctiptors

slozier · 2024-12-08T03:30:44Z

Src/IronPython.Modules/nt.cs

+            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


typo: UnxiStream

slozier · 2024-12-08T03:45:32Z

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")


I guess my brain doesn't understand ab+ mode. 😄 Played around trying to make sense of it, but just ended up finding a behaviour that diverged from CPython...

with open(self.temp_file, "ab+") as f: f.write(b"abcdef") f.seek(0) f.read(2) f.write(b"z") f.seek(0) self.assertEqual(f.read(), b"abzdef")

I also do not fully understand the ab+ mode, so I was playing around with some samples and tests to get a better grip on it; this test being one of them. So this change actually does not belong to this PR, kind of sneaked through from those experiments into the commit. It doesn't hurt though, the more test coverage the better, so I intend to leave it in, but also intend add more ab+ testing later on.

Another thing I don't understand is why ab+ is implemented using separate read and write streams. The dual-stream case does not play well with having one genuine file descriptor per file, but more worrisome is that I do not see any actual need for dual-streams at all. This worries me because maybe I am overlooking something essential that led the original programmer to use dual streams, hence the extra tests to try to catch something that doesn't fit in my current mental model of ab+. If I don't find anything, the next PR will probably get rid of dual streams.

I guess my brain doesn't understand ab+ mode. 😄 Played around trying to make sense of it, but just ended up finding a behaviour that diverged from CPython...

It looks to me that you have found a bug in CPython. If you write four "z" rather than one, it is being correctly appended at the end. I was able to reproduce the strange behaviour you have found on macOS with various CPython versions, up to 3.12.0. Unrelated to this, but I've found open issues around ab+ in CPython, so it does not surprise me if there are more.

I have read the CPython code dealing with ab+ and it simply adds O_APPEND | O_CREAT for file opening (in addition to the other usual flags) plus an initial seek to the end in case the first operation is read iso write. Further it relays on the OS to support it, which, according to POSIX documentation, should perform seek to end before each write atomically (apparently, non-atomically on Windows). However, using pwrite, it is still possible to write inside a file that is O_APPEND, so maybe this is what is happening for writes less than 4 bytes. Or maybe no OS call is done but CPython just updates some internal buffer as an optimization.

In any case, I propose to disregard all discrepancies and simply always write to the end, using one read/write stream.

Maybe a bug, maybe not. Have to keep in mind that open gives a BufferedRandom so the differences could be related to the buffering implementation. I read a comment somewhere (probably stackoverflow) saying you should always seek between read/write operations otherwise results may vary. More weirdness:

with open(self.temp_file, "ab+") as f: f.write(b"abcdef") f.seek(0) f.read(2) f.write(b"z") f.seek(0) a = f.read() f.seek(0) b = f.read() self.assertEqual(a, b"abzdef") self.assertEqual(b, b"abcdefz")

Definitely feels like a buffering thing.

slozier · 2024-12-08T03:46:48Z

Documentation/file-descriptors.md

+
+It is possible to have the structure above without `FileIO`; for instance when an OS file is opened with one of the low-level functions in `os`, or when an existing FD is duplicated. It is also possible to associate an FD with several `FileIO`. In such cases it is the responsibility of the user code to take care that the FD is closed at the right time.
+
+When FD is duplicated (using `dup` or `dup2`), the associated `StreamBox` is duplicated (there is always a 1-to1 relationship between FD and `StreamBox`), but the underlying `FileStream` objects remain the same, and so are the underlying OS handles. The new FD may be used to create a `FileIO` (or several, just as the original FD). All read/seek/write operations on both descriptors go though the same `FileStream` object and the same OS handle.


typos: 1-to1 and though

BCSharp · 2024-12-09T23:36:58Z

Thanks for the review; I put all those typos in there to see is anyone is reading my essays… 😃 …no, I'm just joking.

The PR is draft because I only tested it on Linux and wanted to get the branch over onto my macOS machine. I had no additional work planned, otherwise it was "ready for review". The net465 test on macOS fails, but I don't see any connection with the changes I made. Unfortunately, it also fails locally, for unrelated reasons.

BCSharp added 3 commits December 4, 2024 21:34

Use genuine file descriptors on Unix

827caee

Complete implementing genuine file descriptors

20b0669

Add developer's deocumentation

a2333c4

slozier reviewed Dec 8, 2024

View reviewed changes

BCSharp added 2 commits December 9, 2024 21:28

Update after review

cc92235

Add wbplus file test

380ab36

BCSharp marked this pull request as ready for review December 10, 2024 19:37

slozier merged commit f971ff6 into IronLanguages:main Dec 12, 2024
5 of 8 checks passed

BCSharp deleted the fileno2 branch December 12, 2024 03:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use genuine file descriptors on Posix #1832

Use genuine file descriptors on Posix #1832

BCSharp commented Dec 7, 2024 •

edited

Loading

slozier left a comment

slozier Dec 8, 2024

slozier Dec 8, 2024

BCSharp Dec 9, 2024

slozier Dec 10, 2024

slozier Dec 8, 2024

BCSharp Dec 9, 2024

BCSharp Dec 9, 2024

slozier Dec 8, 2024

slozier Dec 8, 2024

slozier Dec 8, 2024

BCSharp Dec 9, 2024

BCSharp Dec 10, 2024

slozier Dec 10, 2024

slozier Dec 8, 2024

BCSharp commented Dec 9, 2024


		In this way, the file descriptor on the `PythonFileManager` level is the same as the file descriptor used by `FileStream`.

		Unfortunately, on .NET, somehow, the two independent read/write positions. This is not how duplicated file descriptors should work: both descriptors should point to the same file description structure and share the read/seek/write position. In practice, on .NET, writing through the second file object will overwrite data already written through the first file object. In regular Unix applications (incl. CPython), the subsequent writes append data, regardless which file object is used. The same principle should apply to reads.


		It is possible to have the structure above without `FileIO`; for instance when an OS file is opened with one of the low-level functions in `os`, or when an existing FD is duplicated. It is also possible to associate an FD with several `FileIO`. In such cases it is the responsibility of the user code to take care that the FD is closed at the right time.

		When FD is duplicated (using `dup` or `dup2`), the associated `StreamBox` is duplicated (there is always a 1-to1 relationship between FD and `StreamBox`), but the underlying `FileStream` objects remain the same, and so are the underlying OS handles. The new FD may be used to create a `FileIO` (or several, just as the original FD). All read/seek/write operations on both descriptors go though the same `FileStream` object and the same OS handle.

Use genuine file descriptors on Posix #1832

Use genuine file descriptors on Posix #1832

Conversation

BCSharp commented Dec 7, 2024 • edited Loading

slozier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BCSharp commented Dec 9, 2024

BCSharp commented Dec 7, 2024 •

edited

Loading