GNOME Bugzilla – Bug 602412
g_file_replace does not restore original file when there is errors while writing
Last modified: 2018-05-24 12:01:41 UTC
This happens in the glib HEAD and downwards. From documentation: http://library.gnome.org/devel/gio/stable/GFile.html#g-file-replace > This will try to replace the file in the safest way possible so that any > errors during the writing will not affect an already existing copy of the > file. For instance, for local files it may write to a temporary file and > then atomically rename over the destination when the stream is closed. Here is some test code to reproduce the bug: #include <gio/gio.h> int main (int argc, char **argv) { g_type_init (); GError *error = NULL; GFile *file = g_file_new_for_path("minifs/hello.txt"); GOutputStream* out = g_file_replace(file, NULL, FALSE, G_FILE_CREATE_NONE, NULL, &error); int i = 0; if (out == NULL) { g_message("Error: %s\n", error->message); g_error_free(error); } GError *err2 = NULL; for (i = 0; i < 1024*1024/4; i++) { err2 = NULL; if (!g_output_stream_write_all(out, "AAAAAAAA", 8, NULL, NULL, &err2)) { g_message("Error 2: %s\n", err2->message); g_error_free(err2); } } if (out) g_object_unref(out); g_object_unref(file); } The contents of "minifs/hello.txt" say: "Hello, World!" Here "minifs" is just a small file system, in which the writing of this will exhaust the space (an error state). And in "minifs/hello.txt" you'll find bunch of A's, rather than "Hello, World!" This bugs other manifestation: https://bugzilla.gnome.org/show_bug.cgi?id=561309 I also attach a patch.
Created attachment 148123 [details] [review] Patch
I understand the logic here, but it represents a strange break of API. Having your new data thrown away because an error ever happened on the stream is not what we currently do, and it's actually sort of strange. Errors could happen for transient reasons and, after fixing those errors, you would expect to be able to continue your write (like maybe you make more space by erasing another file). Your code doesn't erase the error if writes start succeeding again, and I don't think it should. That would really be "too magical" -- the result of closing the file depends, as a side effect, on what happened in previous calls. Weird. The verbage in the documentation refers, I think, to the fact that your program could crash in the middle of the write without resulting in data loss. Continuing to the end of the lifecycle of the OutputStream constitutes success (in some sense), even if errors occured along the way. I'd be willing to support the addition of some sort of g_file_output_stream_abort_replace() call that you can explicitly call to get the effect you want.
(In reply to comment #2) > I understand the logic here, but it represents a strange break of API. Having > your new data thrown away because an error ever happened on the stream is not > what we currently do, and it's actually sort of strange. > > Errors could happen for transient reasons and, after fixing those errors, you > would expect to be able to continue your write (like maybe you make more space > by erasing another file). OK, I see your point. > Your code doesn't erase the error if writes start succeeding again, and I don't > think it should. That would really be "too magical" -- the result of closing > the file depends, as a side effect, on what happened in previous calls. Weird. OK, now I see and agree, maybe this wasn't the intended API for this. But I don't agree that it's "too magical", I think it is needed to do this somehow. > The verbage in the documentation refers, I think, to the fact that your program > could crash in the middle of the write without resulting in data loss. > Continuing to the end of the lifecycle of the OutputStream constitutes success > (in some sense), even if errors occured along the way. Yes. And I think the passage in documentation should be clarified. > I'd be willing to support the addition of some sort of > g_file_output_stream_abort_replace() call that you can explicitly call to get > the effect you want. Yes, would solve the problem more appropriately. I'll try to produce a patch, when I'll have more time.
Yeah, g_file_output_stream_abort_replace() seems like the right approach.
What about having g_file_output_stream_abort() which would close the file and clean up after itself as if there was an error? 2 reasons why I prefer that: 1) I can imagine myself wanting to cleanup from files created with g_file_create(), too. 2) I might have a processing error in my application and want to abort, even though the output stream worked fine the whole time.
I don't oppose a more generic version, but keep in mind that this is all a "best effort" kind of thing. Its not always to abort, even a replace might not be possible to undo if an in-place rewrite was chosed.
Might be good to have the abort call being able to fail so the user can detect this, and perhaps then manually copy back the backup file.
For that to work we also probably need g_file_output_stream_get_backup which gives a GFile for the backup file.
bug 615110 is similar but not exactly the same. There the error is on the read side of a copy, so the write should be aborted.
Yeah, my idea was to have _abort() do _close() + cleanup and if any of that failed, return an error, but still mark the stream as closed. Of course, if an abort is not possible, it should error out with G_IO_ERROR_NOT_SUPPORTED or so (after closing the file). The rationale would be that you call g_file_output_stream_abort() in place of g_output_stream_close() and get exactly what you want.
Any news on the subject? The local implementation of g_file_replace_contents() (and perhaps other) looses data if write fails and make_backup is FALSE (at least when disk is full). As I see the picture, this is directly related to this bug, right? This is a quite grave bug IMHO, since it may lead to data loss.
I might be wrong (and that might be completely unportable) but I think the situation could be work arounded through using something like: GCancellable *c = g_cancellable_new(); g_cancellable_cancel(c); g_output_stream_close(stream, c, NULL); g_object_unref(stream); The cancellation of operation forbids GIO from doing actual replace after closing the file, and it seems to not try anymore. The drawback of this workaround is that temporary files aren't removed.
Is there any progress here as to the best way to do this? Some GNOME applications have open bugs that are caused or are hard to fix due to this deficiency. Eog: https://bugzilla.gnome.org/show_bug.cgi?id=606497 Rhythmbox: https://bugzilla.gnome.org/show_bug.cgi?id=729664 (probably) Other applications either suffer from (unreported?) bugs relating to this, or have custom (possibly buggy) workarounds. It would be nice to have this implemented in GIO. I want to work on this, but I'd like to know what people think is the best way to implement this feature before doing so. :)
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/249.