Skip to content

Commit

Permalink
Update after review
Browse files Browse the repository at this point in the history
  • Loading branch information
BCSharp committed Dec 10, 2024
1 parent a2333c4 commit cc92235
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 11 deletions.
4 changes: 2 additions & 2 deletions Documentation/file-descriptors.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ Conceptually, the relationship between `FD` (a number) and `StreamBox` (a class)

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.
When FD is duplicated (using `dup` or `dup2`), the associated `StreamBox` is duplicated (there is always a 1-to-1 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.

```mermaid
graph LR;
Expand Down Expand Up @@ -52,7 +52,7 @@ FD2(FD2) <--> StreamBox2[StreamBox] --> FileStream2[FileStream] --> FDH2(FD2) --

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.
Unfortunately, on .NET, somehow, two `FileStream` instances using the same file descriptor will have 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.

Also unfortunately, on Mono, the `FileStream` constructor accepts only descriptors opened by another call to a `FileStream` constructor[[1]]. So descriptors obtained from direct OS calls, like `open`, `creat`, `dup`, `dup2` are being rejected.

Expand Down
14 changes: 7 additions & 7 deletions Src/IronPython.Modules/nt.cs
Original file line number Diff line number Diff line change
Expand Up @@ -351,11 +351,7 @@ public static int dup(CodeContext/*!*/ context, int fd) {
PythonFileManager fileManager = context.LanguageContext.FileManager;

StreamBox streams = fileManager.GetStreams(fd); // OSError if fd not valid
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) {
fileManager.EnsureRefStreams(streams);
fileManager.AddRefStreams(streams);
return fileManager.Add(new(streams));
} else {
if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) {
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;
Expand All @@ -369,6 +365,10 @@ public static int dup(CodeContext/*!*/ context, int fd) {
fileManager.AddRefStreams(streams);
return fileManager.Add(fd2, new(streams));
}
} else {
fileManager.EnsureRefStreams(streams);
fileManager.AddRefStreams(streams);
return fileManager.Add(new(streams));
}
}

Expand All @@ -389,7 +389,7 @@ 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 (simulated desctiptors only)
// TODO: race condition: `open` or `dup` on another thread may occupy fd2 (simulated descriptors only)

if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) {
fileManager.EnsureRefStreams(streams);
Expand Down Expand Up @@ -418,7 +418,7 @@ 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
// This does not work on .NET, probably because .NET FileStream is not aware of Mono.Unix.UnixStream
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
Expand Down
4 changes: 2 additions & 2 deletions Src/IronPython/Runtime/PythonFileManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -234,8 +234,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")]
// Must not be used on POSIX.
[UnsupportedOSPlatform("linux"), UnsupportedOSPlatform("macos")]
public int Add(StreamBox streams) {
ContractUtils.RequiresNotNull(streams, nameof(streams));
ContractUtils.Requires(streams.Id < 0, nameof(streams));
Expand Down

0 comments on commit cc92235

Please sign in to comment.