Skip to content

Commit 05cfb0e

Browse files
BCSharpBCSharp
authored
Support mode ab+ equivalent through os.open (#1839)
* Use dual stream for os.open with O_APPEND * Maintain file position when accessing file descriptor * Add test * Fix typo
1 parent 9358e4b commit 05cfb0e

File tree

4 files changed

+72
-23
lines changed

4 files changed

+72
-23
lines changed

‎Src/IronPython.Modules/nt.cs

Lines changed: 30 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -352,10 +352,6 @@ public static int dup(CodeContext/*!*/ context, int fd) {
352352

353353
StreamBox streams = fileManager.GetStreams(fd); // OSError if fd not valid
354354
if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) {
355-
if (!streams.IsSingleStream && fd is 1 or 2) {
356-
// If there is a separate write stream, dupping over stout or sderr uses write stream's file descriptor
357-
fd = streams.WriteStream is FileStream fs ? fs.SafeFileHandle.DangerousGetHandle().ToInt32() : fd;
358-
}
359355
int fd2 = UnixDup(fd, -1, out Stream? dupstream);
360356
if (dupstream is not null) {
361357
return fileManager.Add(fd2, new(dupstream));
@@ -389,16 +385,14 @@ public static int dup2(CodeContext/*!*/ context, int fd, int fd2) {
389385
close(context, fd2);
390386
}
391387

392-
// TODO: race condition: `open` or `dup` on another thread may occupy fd2 (simulated descriptors only)
388+
// TODO: race condition: `open` or `dup` on another thread may occupy fd2
393389

394-
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) {
395-
fileManager.EnsureRefStreams(streams);
396-
fileManager.AddRefStreams(streams);
397-
return fileManager.Add(fd2, new(streams));
398-
} else {
399-
if (!streams.IsSingleStream && fd is 1 or 2) {
400-
// If there is a separate write stream, dupping over stout or sderr uses write stream's file descriptor
401-
fd = streams.WriteStream is FileStream fs ? fs.SafeFileHandle.DangerousGetHandle().ToInt32() : fd;
390+
if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) {
391+
if (!streams.IsSingleStream && fd2 is 0 && streams.ReadStream is FileStream fs) {
392+
// If there is a separate read stream, dupping over stdin uses read stream's file descriptor as source
393+
long pos = fs.Position;
394+
fd = fs.SafeFileHandle.DangerousGetHandle().ToInt32();
395+
fs.Seek(pos, SeekOrigin.Begin);
402396
}
403397
fd2 = UnixDup(fd, fd2, out Stream? dupstream); // closes fd2 atomically if reopened in the meantime
404398
fileManager.Remove(fd2);
@@ -410,6 +404,10 @@ public static int dup2(CodeContext/*!*/ context, int fd, int fd2) {
410404
fileManager.AddRefStreams(streams);
411405
return fileManager.Add(fd2, new(streams));
412406
}
407+
} else {
408+
fileManager.EnsureRefStreams(streams);
409+
fileManager.AddRefStreams(streams);
410+
return fileManager.Add(fd2, new(streams));
413411
}
414412
}
415413

@@ -877,8 +875,9 @@ public static object open(CodeContext/*!*/ context, [NotNone] string path, int f
877875
FileMode fileMode = FileModeFromFlags(flags);
878876
FileAccess access = FileAccessFromFlags(flags);
879877
FileOptions options = FileOptionsFromFlags(flags);
880-
Stream s;
881-
FileStream? fs;
878+
Stream s; // the stream opened to acces the file
879+
FileStream? fs; // downcast of s if s is FileStream (this is always the case on POSIX)
880+
Stream? rs = null; // secondary read stream if needed, otherwise same as s
882881
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && IsNulFile(path)) {
883882
fs = null;
884883
s = Stream.Null;
@@ -889,15 +888,28 @@ public static object open(CodeContext/*!*/ context, [NotNone] string path, int f
889888
fs.Close();
890889
s = fs = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.ReadWrite, DefaultBufferSize, options);
891890
} else if (access == FileAccess.ReadWrite && fileMode == FileMode.Append) {
891+
// .NET doesn't allow Append w/ access != Write, so open the file w/ Write
892+
// and a secondary stream w/ Read, then seek to the end.
892893
s = fs = new FileStream(path, FileMode.Append, FileAccess.Write, FileShare.ReadWrite, DefaultBufferSize, options);
894+
rs = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.ReadWrite, DefaultBufferSize, options);
895+
rs.Seek(0L, SeekOrigin.End);
893896
} else {
894897
s = fs = new FileStream(path, fileMode, access, FileShare.ReadWrite, DefaultBufferSize, options);
895898
}
899+
rs ??= s;
896900

897-
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) {
898-
return context.LanguageContext.FileManager.Add(new(s));
901+
if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) {
902+
int fd = fs!.SafeFileHandle.DangerousGetHandle().ToInt32();
903+
// accessing SafeFileHandle may reset file position
904+
if (fileMode == FileMode.Append) {
905+
fs.Seek(0L, SeekOrigin.End);
906+
}
907+
if (!ReferenceEquals(fs, rs)) {
908+
rs.Seek(fs.Position, SeekOrigin.Begin);
909+
}
910+
return context.LanguageContext.FileManager.Add(fd, new(rs, s));
899911
} else {
900-
return context.LanguageContext.FileManager.Add((int)fs!.SafeFileHandle.DangerousGetHandle(), new(s));
912+
return context.LanguageContext.FileManager.Add(new(rs, s));
901913
}
902914
} catch (Exception e) {
903915
throw ToPythonException(e, path);

‎Src/IronPython/Modules/_fileio.cs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,19 @@ public FileIO(CodeContext/*!*/ context, string name, string mode = "r", bool clo
127127
default:
128128
throw new InvalidOperationException();
129129
}
130+
if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) {
131+
// On POSIX, register the file descriptor with the file manager right after file opening
132+
_context.FileManager.GetOrAssignId(_streams);
133+
// according to [documentation](https://learn.microsoft.com/en-us/dotnet/api/system.io.filestream.safefilehandle?view=net-9.0#remarks)
134+
// accessing SafeFileHandle sets the current stream position to 0
135+
// in practice it doesn't seem to be the case, but better to be sure
136+
if (this.mode == "ab+") {
137+
_streams.WriteStream.Seek(0L, SeekOrigin.End);
138+
}
139+
if (!_streams.IsSingleStream) {
140+
_streams.ReadStream.Seek(_streams.WriteStream.Position, SeekOrigin.Begin);
141+
}
142+
}
130143
}
131144
else {
132145
object fdobj = PythonOps.CallWithContext(context, opener, name, flags);

‎Src/IronPython/Runtime/PythonFileManager.cs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -287,12 +287,12 @@ public int GetOrAssignId(StreamBox streams) {
287287
lock (_synchObject) {
288288
int res = streams.Id;
289289
if (res == -1) {
290-
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) {
291-
res = Add(streams);
292-
} else {
293-
res = GetGenuineFileDescriptor(streams.ReadStream);
290+
if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) {
291+
res = GetGenuineFileDescriptor(streams.WriteStream);
294292
if (res < 0) throw new InvalidOperationException("stream not associated with a file descriptor");
295293
Add(res, streams);
294+
} else {
295+
res = Add(streams);
296296
}
297297
}
298298
return res;
@@ -327,7 +327,7 @@ public bool ValidateFdRange(int fd) {
327327
return fd >= 0 && fd < LIMIT_OFILE;
328328
}
329329

330-
[UnsupportedOSPlatform("windows")]
330+
[SupportedOSPlatform("linux"), SupportedOSPlatform("macos")]
331331
private static int GetGenuineFileDescriptor(Stream stream) {
332332
return stream switch {
333333
FileStream fs => checked((int)fs.SafeFileHandle.DangerousGetHandle()),

‎Tests/modules/system_related/test_os.py

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,14 @@
77
from iptest import IronPythonTestCase, is_osx, is_linux, is_windows, run_test
88

99
class OsTest(IronPythonTestCase):
10+
def setUp(self):
11+
super(OsTest, self).setUp()
12+
self.temp_file = os.path.join(self.temporary_dir, "temp_OSTest_%d.dat" % os.getpid())
13+
14+
def tearDown(self):
15+
self.delete_files(self.temp_file)
16+
return super().tearDown()
17+
1018
def test_strerror(self):
1119
if is_windows:
1220
self.assertEqual(os.strerror(0), "No error")
@@ -29,4 +37,20 @@ def test_strerror(self):
2937
elif is_osx:
3038
self.assertEqual(os.strerror(40), "Message too long")
3139

40+
def test_open_abplus(self):
41+
# equivalent to open(self.temp_file, "ab+"), see also test_file.test_open_abplus
42+
fd = os.open(self.temp_file, os.O_APPEND | os.O_CREAT | os.O_RDWR)
43+
try:
44+
f = open(fd, mode="ab+", closefd=False)
45+
f.write(b"abc")
46+
f.seek(0)
47+
self.assertEqual(f.read(2), b"ab")
48+
f.write(b"def")
49+
self.assertEqual(f.read(2), b"")
50+
f.seek(0)
51+
self.assertEqual(f.read(6), b"abcdef")
52+
f.close()
53+
finally:
54+
os.close(fd)
55+
3256
run_test(__name__)

0 commit comments

Comments
 (0)