search

nim-lang / Nim

Nim is a statically typed compiled systems programming language. It combines successful concepts from mature languages like Python, Ada and Modula. Its design focuses on efficiency, expressiveness, and elegance (in that order of priority).
https://nim-lang.org
Other
16.22k stars 1.46k forks source link

Fix non-exported `memfiles.setFileSize` to be able to shrink files on posix via `memfiles.resize` #23717

Closed c-blake closed 2 weeks ago

c-blake commented 2 weeks ago

Fix non-exported setFileSize to take optional oldSize to (on posix) shrink differently than it grows (ftruncate not posix_fallocate) since it makes sense to assume the higher address space has already been allocated there and include the old file size in the proc resize call. Also, do not even try setFileSize in the first place unless the open itself works by moving the call into the if newFileSize != -1 branch.

Just cosmetics, also improve some old 2011 comments, note a logic diff for callers using both mappedSize & newFileSize from windows branch in case someone wants to fix that & simplify code formatting a little.

Araq commented 2 weeks ago

Windows support would be nice if Windows supports it without too much effort.

github-actions[bot] commented 2 weeks ago

Thanks for your hard work on this PR! The lines below are statistics of the Nim compiler built from 8037bbe327ec581fbafd558bf1eb4619d373e7ec

Hint: mm: orc; opt: speed; options: -d:release 178758 lines; 8.369s; 664.242MiB peakmem

c-blake commented 2 weeks ago

Thanks for the quick merge.

On Windows the setFilePointer()-setEndOfFile() pair should already do the right thing in both the shrink & grow cases. So, there is no bug in that branch or any need for special support - it should already work due to OS semantics.

In more detail, the bug only happens because posix_fallocate will both grow capacity and grow address space backed by that new reservation. So, if growing you do not need the ftruncate, but if shrinking that is all you need { like setLen(var seq[T]) down but not up }. It was simply neglected here prior to this PR { which mostly just adds an else: branch (to a new if:) for that with a needed parameter and then adjustments at the callers }. So, files never actually shrank on Unix, but they already did on Windows. I should maybe have filed an issue explaining this before just fixing it in the PR.

In yet more detail, I have test code over at https://github.com/c-blake/cligen/blob/f9c8b960ab07e83bde63d8ea790925708db0c981/cligen/osUt.nim#L589-L605 that runs fine on a virtual machine with Windows MSYS2. I did not include that here because, in lib/pure/memfiles.nim, setFileSize is not an exported API call and further if there is sufficient interest to make it exported then setFileSize should probably live in some other file and be imported by memfiles, since it is not only about memfiles. (There was already text in the doc comment suggesting it could move to std/osfiles.)

The bug here was actually discovered in my automagic identifier bike shedder which uses std/memfiles for a custom-built efficient "database/datafile" which does not quite work on Windows failing with an error syncio.nim(155) raiseEIO Error: unhandled exception: error: 1 Incorrect function. [IOError]. { That failure is the same before & after this PR and is unrelated to the fixes in the posix branches here. On posix thes now works just as it should. }