After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 602412 - g_file_replace does not restore original file when there is errors while writing
g_file_replace does not restore original file when there is errors while writing
Status: RESOLVED OBSOLETE
Product: glib
Classification: Platform
Component: gio
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2009-11-19 14:21 UTC by Ramūnas Gutkovas
Modified: 2018-05-24 12:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch (2.59 KB, patch)
2009-11-19 14:22 UTC, Ramūnas Gutkovas
none Details | Review

Description Ramūnas Gutkovas 2009-11-19 14:21:04 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.
Comment 1 Ramūnas Gutkovas 2009-11-19 14:22:14 UTC
Created attachment 148123 [details] [review]
Patch
Comment 2 Allison Karlitskaya (desrt) 2009-11-20 15:21:37 UTC
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.
Comment 3 Ramūnas Gutkovas 2009-11-20 18:45:16 UTC
(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.
Comment 4 Alexander Larsson 2009-11-26 14:37:33 UTC
Yeah, g_file_output_stream_abort_replace() seems like the right approach.
Comment 5 Benjamin Otte (Company) 2010-04-08 18:57:12 UTC
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.
Comment 6 Alexander Larsson 2010-04-08 19:06:12 UTC
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.
Comment 7 Alexander Larsson 2010-04-08 19:07:42 UTC
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.
Comment 8 Alexander Larsson 2010-04-08 19:09:01 UTC
For that to work we also probably need g_file_output_stream_get_backup which gives a GFile for the backup file.
Comment 9 Alexander Larsson 2010-04-08 19:10:52 UTC
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.
Comment 10 Benjamin Otte (Company) 2010-04-08 19:14:36 UTC
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.
Comment 11 Colomban Wendling 2010-11-05 23:21:43 UTC
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.
Comment 12 Michał Górny 2011-03-12 09:22:05 UTC
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.
Comment 13 Nirbheek Chauhan 2014-06-03 15:08:21 UTC
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. :)
Comment 14 GNOME Infrastructure Team 2018-05-24 12:01:41 UTC
-- 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.