Skip to content

Commit

Permalink
Support mode ab+ equivalent through os.open (#1839)
Browse files Browse the repository at this point in the history 
* Use dual stream for os.open with O_APPEND

* Maintain file position when accessing file descriptor

* Add test

* Fix typo
  • Loading branch information
@BCSharp
BCSharp authored Dec 13, 2024
1 parent 9358e4b commit 05cfb0e
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 23 deletions.
48 changes: 30 additions & 18 deletions Src/IronPython.Modules/nt.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -352,10 +352,6 @@ public static int dup(CodeContext/*!*/ context, int fd) {

StreamBox streams = fileManager.GetStreams(fd); // OSError if fd not valid
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;
}
int fd2 = UnixDup(fd, -1, out Stream? dupstream);
if (dupstream is not null) {
return fileManager.Add(fd2, new(dupstream));
Expand Down Expand Up @@ -389,16 +385,14 @@ 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 descriptors only)
// TODO: race condition: `open` or `dup` on another thread may occupy fd2

if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) {
fileManager.EnsureRefStreams(streams);
fileManager.AddRefStreams(streams);
return fileManager.Add(fd2, new(streams));
} 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;
if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) {
if (!streams.IsSingleStream && fd2 is 0 && streams.ReadStream is FileStream fs) {
// If there is a separate read stream, dupping over stdin uses read stream's file descriptor as source
long pos = fs.Position;
fd = fs.SafeFileHandle.DangerousGetHandle().ToInt32();
fs.Seek(pos, SeekOrigin.Begin);
}
fd2 = UnixDup(fd, fd2, out Stream? dupstream); // closes fd2 atomically if reopened in the meantime
fileManager.Remove(fd2);
Expand All @@ -410,6 +404,10 @@ public static int dup2(CodeContext/*!*/ context, int fd, int fd2) {
fileManager.AddRefStreams(streams);
return fileManager.Add(fd2, new(streams));
}
} else {
fileManager.EnsureRefStreams(streams);
fileManager.AddRefStreams(streams);
return fileManager.Add(fd2, new(streams));
}
}

Expand Down Expand Up @@ -877,8 +875,9 @@ public static object open(CodeContext/*!*/ context, [NotNone] string path, int f
FileMode fileMode = FileModeFromFlags(flags);
FileAccess access = FileAccessFromFlags(flags);
FileOptions options = FileOptionsFromFlags(flags);
Stream s;
FileStream? fs;
Stream s; // the stream opened to acces the file
FileStream? fs; // downcast of s if s is FileStream (this is always the case on POSIX)
Stream? rs = null; // secondary read stream if needed, otherwise same as s
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && IsNulFile(path)) {
fs = null;
s = Stream.Null;
Expand All @@ -889,15 +888,28 @@ public static object open(CodeContext/*!*/ context, [NotNone] string path, int f
fs.Close();
s = fs = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.ReadWrite, DefaultBufferSize, options);
} else if (access == FileAccess.ReadWrite && fileMode == FileMode.Append) {
// .NET doesn't allow Append w/ access != Write, so open the file w/ Write
// and a secondary stream w/ Read, then seek to the end.
s = fs = new FileStream(path, FileMode.Append, FileAccess.Write, FileShare.ReadWrite, DefaultBufferSize, options);
rs = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.ReadWrite, DefaultBufferSize, options);
rs.Seek(0L, SeekOrigin.End);
} else {
s = fs = new FileStream(path, fileMode, access, FileShare.ReadWrite, DefaultBufferSize, options);
}
rs ??= s;

if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) {
return context.LanguageContext.FileManager.Add(new(s));
if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) {
int fd = fs!.SafeFileHandle.DangerousGetHandle().ToInt32();
// accessing SafeFileHandle may reset file position
if (fileMode == FileMode.Append) {
fs.Seek(0L, SeekOrigin.End);
}
if (!ReferenceEquals(fs, rs)) {
rs.Seek(fs.Position, SeekOrigin.Begin);
}
return context.LanguageContext.FileManager.Add(fd, new(rs, s));
} else {
return context.LanguageContext.FileManager.Add((int)fs!.SafeFileHandle.DangerousGetHandle(), new(s));
return context.LanguageContext.FileManager.Add(new(rs, s));
}
} catch (Exception e) {
throw ToPythonException(e, path);
Expand Down
13 changes: 13 additions & 0 deletions Src/IronPython/Modules/_fileio.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,19 @@ public FileIO(CodeContext/*!*/ context, string name, string mode = "r", bool clo
default:
throw new InvalidOperationException();
}
if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) {
// On POSIX, register the file descriptor with the file manager right after file opening
_context.FileManager.GetOrAssignId(_streams);
// according to [documentation](https://learn.microsoft.com/en-us/dotnet/api/system.io.filestream.safefilehandle?view=net-9.0#remarks)
// accessing SafeFileHandle sets the current stream position to 0
// in practice it doesn't seem to be the case, but better to be sure
if (this.mode == "ab+") {
_streams.WriteStream.Seek(0L, SeekOrigin.End);
}
if (!_streams.IsSingleStream) {
_streams.ReadStream.Seek(_streams.WriteStream.Position, SeekOrigin.Begin);
}
}
}
else {
object fdobj = PythonOps.CallWithContext(context, opener, name, flags);
Expand Down
10 changes: 5 additions & 5 deletions Src/IronPython/Runtime/PythonFileManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,12 +287,12 @@ public int GetOrAssignId(StreamBox streams) {
lock (_synchObject) {
int res = streams.Id;
if (res == -1) {
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) {
res = Add(streams);
} else {
res = GetGenuineFileDescriptor(streams.ReadStream);
if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) {
res = GetGenuineFileDescriptor(streams.WriteStream);
if (res < 0) throw new InvalidOperationException("stream not associated with a file descriptor");
Add(res, streams);
} else {
res = Add(streams);
}
}
return res;
Expand Down Expand Up @@ -327,7 +327,7 @@ public bool ValidateFdRange(int fd) {
return fd >= 0 && fd < LIMIT_OFILE;
}

[UnsupportedOSPlatform("windows")]
[SupportedOSPlatform("linux"), SupportedOSPlatform("macos")]
private static int GetGenuineFileDescriptor(Stream stream) {
return stream switch {
FileStream fs => checked((int)fs.SafeFileHandle.DangerousGetHandle()),
Expand Down
24 changes: 24 additions & 0 deletions Tests/modules/system_related/test_os.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,14 @@
from iptest import IronPythonTestCase, is_osx, is_linux, is_windows, run_test

class OsTest(IronPythonTestCase):
def setUp(self):
super(OsTest, self).setUp()
self.temp_file = os.path.join(self.temporary_dir, "temp_OSTest_%d.dat" % os.getpid())

def tearDown(self):
self.delete_files(self.temp_file)
return super().tearDown()

def test_strerror(self):
if is_windows:
self.assertEqual(os.strerror(0), "No error")
Expand All @@ -29,4 +37,20 @@ def test_strerror(self):
elif is_osx:
self.assertEqual(os.strerror(40), "Message too long")

def test_open_abplus(self):
# equivalent to open(self.temp_file, "ab+"), see also test_file.test_open_abplus
fd = os.open(self.temp_file, os.O_APPEND | os.O_CREAT | os.O_RDWR)
try:
f = open(fd, mode="ab+", closefd=False)
f.write(b"abc")
f.seek(0)
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")
f.close()
finally:
os.close(fd)

run_test(__name__)

0 comments on commit 05cfb0e

Please sign in to comment.