Save to temporary file and rename to prevent file corruption during file write

[Moult]

A user reported a crash during save and the IFC had all sorts of unreferenced STEP IDs. After discussion with @brunopostle this seems to be a safer approach.

[aothms]

This makes sense. Just make sure it's on the same partition so that the move is atomic. So instead of tmpdir I'd recommend adding a postfix like my_file.ifc.randomnumber123.tmp. Feel free to implement it under ifcopenshell.file.write() with a keyword arg to turn it on or off (default I don't care).

[Andrej730]

That sounds like a very good idea.

Ryan recently reported an issue saving .blend file to dropbox (I guess it occurs when dropbox is about to sync and it's locking it's folder).

And what I've noticed investigating it, Blender is might be using a similar approach - saving data to temporary file first and then renaming it:

[Moult]

Bump @brunopostle :)

[brunopostle]

I haven't forgotten, just need to find the time

[brunopostle]

An update on this, I can implement atomic file writes in ifcopenshell.file.write() with something like this:

        if atomic:
            fd, temp_path = tempfile.mkstemp(dir=path.parent)
            temp_path = Path(temp_path)
            self.wrapped_data.write(str(temp_path))
            temp_path.replace(path)
        else:
            self.wrapped_data.write(str(path))

I also need to fiddle with the file permissions, but the main problem is that the IFC file gets written with the FILE_NAME header set to the temporary file name, and there is no way to fix this unless I put the temporary file in a different folder.

[brunopostle]

Some more research: it would be easy to create a temporary folder, write the file there, move it to the parent folder, and delete the temporary folder.

This would be atomic on ext4, NTFS and APFS. However it isn't atomic on fat32 or SMB, so this technique would be a problem on USB sticks and network drives.

Alternatives I can see:

1. Rename the old file to myfile.bak, write the file to the correct filename, optionally delete the .bak file. This is what autocad does, and I guess this is what all those .blend1 files left around by blender are for.
2. Change the c++ to allow setting the FILE_NAME header to something other than the filename.

[aothms]

Change the c++ to allow setting the FILE_NAME header to something other than the filename.

The file_name is not set on the C++ side of things. It's not even really known in the library because we open the filestream in swig and stream the C++ file object through that.

IfcOpenShell/src/ifcwrap/IfcParseWrapper.i

Lines 143 to 146 in 6d94871

	void write(const std::string& fn) {
	std::ofstream f(IfcUtil::path::from_utf8(fn).c_str());
	f << (*$self);
	}

[Andrej730]

An update on this, I can implement atomic file writes in ifcopenshell.file.write() with something like this:

        if atomic:
            fd, temp_path = tempfile.mkstemp(dir=path.parent)
            temp_path = Path(temp_path)
            self.wrapped_data.write(str(temp_path))
            temp_path.replace(path)
        else:
            self.wrapped_data.write(str(path))

Looks good in general but on Windows it seems to require os.close(fd) before .replace, otherwise I get the error below with the snippet

import ifcopenshell
from pathlib import Path

path = Path(r"C:\") / "project.ifc"
ifc_file = ifcopenshell.file()
ifc_file.write(path)
print('finished')

Error:

Traceback (most recent call last):
  File "\test.py", line 6, in <module>
    ifc_file.write(path)
  File "\ifcopenshell\file.py", line 684, in write
    temp_path.replace(path)
  File "\Lib\pathlib.py", line 1188, in replace
    os.replace(self, target)
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\tmp8nl2f1c6' -> 'C:\\project.ifc

Also about permissions. Was investigating #5809 and found that on Windows (not sure how it is on Unix) os.access(dir_path, os.W_OK) is unreliable as it returns True even if user doesn't have write access to the directory. So the ultimate test seems to be to just create a file to see if it works.

So in theory tempfile.mkstemp should have cover that but there's a bug in tempfile (python/cpython#66305 (comment), see some workaround in the linked comment) when it doesn't recognize missing permissions and just keeps hanging indefinetely until it tries all zillion names for temporary files... So, I guess the solution is to create our method for creating a temp file? :\

[aothms]

Looks good in general but on Windows it seems to require os.close(fd) before .replace

Yes that's what I recall as well, linux allows to delete/move a file that's used, but windows doesn't.

So the ultimate test seems to be to just create a file to see if it works.

That's what I recall as best practice as well

So, I guess the solution is to create our method for creating a temp file? :\

Shouldn't we just implement this fully in C++. Just always write to my_file.ifc~ for example and at the very end rename transparently in (swig generated) C++.

Note that in IfcConvert we already do sth similar.

IfcOpenShell/src/ifcconvert/IfcConvert.cpp

Line 622 in c9847ca

path_t output_temp_filename = output_filename + IfcUtil::path::from_utf8(TEMP_FILE_EXTENSION);