Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support mode ab+ equivalent through os.open #1839

Merged
merged 4 commits into from
Dec 13, 2024

Conversation

BCSharp
Copy link
Member

@BCSharp BCSharp commented Dec 12, 2024

I think I understand now why IronPython uses sometimes two streams for accessing one file. I may be wrong, but this is what I think is the case:

  1. CPython was build on libc, which supports various file opening modes according to POSIX. O_APPEND | O_CREAT | O_RDWR is one of them, tagged as mode ab+.
  2. .NET Framework was build on Win32, which does not support file mode Append with read access.
  3. When CPython is ported to Win32, it uses MSVCRT, which emulates Append with R/W by opening with a regular Open and then seeking to the end before every write. This is functionally the same as on POSIX but non-atomic, so it may lead to data loss in some circumstances.
  4. When .NET was re-developed on POSIX, it maintained the limitation from Win32 of not supporting R/W with Append, perhaps for consistency of API.
  5. IronPython emulates ab+ by opening two streams, one for writing with proper file mode Append, and a secondary one for reading. Unlike MSVCRT, it does not suffer with problems with data overwrite.
  6. Two streams per file work OK-ish on POSIX, but cause complications when genuine file descriptors come into play (since there are now two open descriptors per one file).

Given above, I decided to keep the two-stream solution, which seems to me is optimal for Win32. In this PR, I extend the two-stream solution to os.open (see added test, which is now passing). The usual workarounds for POSIX are still in place, but I still hope that for POSIX, ab+ can be implemented with one stream — something for another PR.

Other changes in this PR is an explicit set of file position after accessing SafeFileHandle, which, according to the documentation should reset the position to 0. I have not observed this behaviour in the few tests I have run (on macOS/linux the position was unchanged), but I'd rather not argue with the documentation. Perhaps there are some situations when the position is moved. Therefore, SafeFileHandle is now only accessed right after opening, before the descriptor/file is passed to the caller (with one exception of dup2, which may not even survive the winter).

Finally, wherever I see it, I change the code to put the "platform agnostic" branch in the else clause.

Copy link
Contributor

@slozier slozier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me. Only spotted one typo this time (accoding). 😉

@BCSharp BCSharp merged commit 05cfb0e into IronLanguages:main Dec 13, 2024
3 of 8 checks passed
@BCSharp BCSharp deleted the osopen_abplus branch December 13, 2024 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants