File destructor alters mtime/ctime

[supermihi]

A user of pytaglib pointed out the following issue to me, which is actually inherited from taglib itself:

Changing tags of a file and then calling save() will, as expected, immediately write the changes to disk and thereby alter the file's stats. However, once the File object is destroyed, the destructor of File deletes the stream attribute of FilePrivate, which in turn closes the FileStream, which modifies the stats again. So, the mtime and ctime stats of the file won't match the time save() was called, but instead a later, potentially unpredictable (in case of garbage collection) moment.

As this is definitely not the expected behavior, I would consider it a bug. However I'm afraid that it's not easy to fix, because the File implementation heavily relies on stream being open, right?

[TsudaKageyu]

Just a quick thought, if you can change the pytaglib's API, how about adding close() method which deletes _f to pytaglib's File class?

[supermihi]

Yes, that's exactly what the user that reported the bug to be suggested. However, in upstream taglib the issue would remain - I think a close() method in Taglib::File itself would make sense too, wouldn't it?

[TsudaKageyu]

IMO, it's logical that TagLib and pytaglib have different interfaces, since C++ and python are totally different animals. In C++, we can control the timing of destructor invocation.
Actually, it's easy to add File::close() method. But I'm not sure it will be useful. I'd like to ask some people for their opinions.

[lalinsky]

I don't really understand what is causing the mtime/ctime to change.

[supermihi]

@lalinsky As far as I understand, it's the del file which then closes the stream, issued explicitly in C++ and in File.__del__ in my Python API.

But I agree with @TsudaKageyu that it's okay to have File.close() in Python and not in C++, so I'll do that. Adding File::close() in C++ would probably involve a lot of changes because, at a first glance, tons of methods in File and its subclasses rely on the stream being open when called.

[sbooth]

I assume that mtime/ctime are being modified by the call to fflush via fclose in FileStream::~FileStream() in the case where there are buffered but unwritten changes to the file handle. This got me thinking a bit and I wonder if it makes sense to add a flush() method to File that subclasses should be responsible for calling at the completion of save().

[TsudaKageyu]

@sbooth, good catch! It's worth it to try.
However, we have to add a virtual function to IOStream or use some workaround to do it. I think it's fine to try it in taglib2.

[supermihi]

I also approve flushing the stream in save(). The mtime/ctime issue however would probably remain, since fclose would still need to be called in FileStream::~FileStreaim(), I think?

[TsudaKageyu]

Right, we need an experiment to check if it works.

[supermihi]

I made this experiment, implementing IOStream::flush() and calliing it in FLAC::File::save(). Unfortunatelly, this has no effect on the mtime/ctime behavior.

[supermihi]

FYI, the current pytaglib version 1.1.0 (supermihi/pytaglib@f9754bd) implements a close() method.