251091 - add Finish() method to nsISafeFileOutputStream

Status: RESOLVED FIXED

(Core :: Networking: File, defect)

Description

[[e-mail]]

this is a spinoff from bug 246675, where we ended up floundering around with
rv-propagation issues.

* darin thinks that nsISafeFileInputStream should just have a Finish() method.
<dwitte> heh
<darin> no writeSucceeded madness
<dwitte> what would that do?
<darin> call Finish to indicate that you are done writing, and want the file to
be moved to the target location.
<darin> if you do not call Finish, then the file will not be saved, and the
tempfile will be removed.
* dwitte likes
<darin> the caller can then also check the return value of Finish to find out if
the file was saved properly
<dwitte> actually...yeah. that would solve the problem of trying to pack too
much into Close
<darin> right
<darin> and the caller could then ignore errors from Write if they wanted
because we'd still trap them in Finish

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

sounds good. just to be clear, Finish would presumably be called after Close(),
right? (so that the file descriptor is closed and the file can be renamed)

Comment 2

[[e-mail]]

actually, Finish would do the closing itself. It will take care of checking all
the writes succeeded, closing the descriptor, and (re)moving the temp file. Most
importantly, the rv from Finish will reflect whether everything succeeded, for
convenience to the consumer (rather than it having to check several rv's, which
is more ambiguous from an interface usage perspective).

Comment 3

[[e-mail]]

Attachment: v1

as promised. one notable difference here is that we now save the nsresult from
a failed Write() call, and propagate it directly back to the consumer in
Finish().

does this look ok?

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 153024 [details] [diff] [review]
v1

Looks good, although i wonder if a failure in close() might be temporary, like
when the disk is still writing or something.

And shouldn't the two consumers be updated to retry saving when writing failed?

r=mvl with that fixed (or good arguments against it :) )

Comment 5

[Darin Fisher]

> Looks good, although i wonder if a failure in close() might be temporary, like
> when the disk is still writing or something.

From "man close" on Fedora Core 2:

       Not  checking  the  return  value of close is a common but nevertheless
       serious programming error.  It is quite possible that errors on a  pre-
       vious  write(2)  operation  are first reported at the final close.  Not
       checking the return value when closing the file may lead to silent loss
       of data.  This can especially be observed with NFS and disk quotas.
                                                                               
                   
       A  successful  close does not guarantee that the data has been success-
       fully saved to disk, as the kernel defers writes. It is not common  for
       a  filesystem  to  flush  the buffers when the stream is closed. If you
       need to be sure that the data is physically stored use  fsync(2).   (It
       will depend on the disk hardware at this point.)

Perhaps we should call PR_Sync before calling PR_Close.

 
> And shouldn't the two consumers be updated to retry saving when writing failed?

Yeah, perhaps... but, I think that's outside the scope of this bug.

Comment 6

[Darin Fisher]

Comment on attachment 153024 [details] [diff] [review]
v1

>Index: netwerk/base/src/nsFileStreams.cpp

> nsSafeFileOutputStream::Write(const char *buf, PRUint32 count, PRUint32 *result)
> {
>     nsresult rv = nsFileOutputStream::Write(buf, count, result);
>+    if (NS_SUCCEEDED(mWriteResult)) {
>+        if (NS_FAILED(rv))
>+            mWriteResult = rv;
>+        else if (count != *result)
>+            mWriteResult = NS_ERROR_FAILURE;
>+    } 
>     return rv;
> }

good idea to verify |count == *result|, but perhaps it'd be good
to add a NS_WARNING in the failure case.  NS_ERROR_UNEXPECTED
might be better than NS_ERROR_FAILURE too.


sr=darin (thanks dwitte!)

Comment 7

[Darin Fisher]

feel free to take the PR_Sync issue to another bug.

Comment 8

[[e-mail]]

Comment on attachment 153024 [details] [diff] [review]
v1

thanks darin, i'll make those changes.

requesting approval for this patch, which revises the API of the safe file
streams code we added around a week ago. it'd be nice to have this change in
before alpha, so we ship with the updated interface, before consumers start
using it.

Comment 9

[Mike Shaver (:shaver emeritus)]

Comment on attachment 153024 [details] [diff] [review]
v1

We have another alpha for this, and 1.8a2 already has one foot out the door. 
Also, Asa would kill me.

Comment 10

[[e-mail]]

needed-aviary1.0 by proxy (bug 250092).

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

+            mWriteResult = NS_ERROR_FAILURE;

would another error code be better? like, NS_ERROR_LOSS_OF_SIGNIFICANT_DATA or
NS_BASE_STREAM_OSERROR?

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

also... could you add a comment in the idl file that Close should never be
called on a safefileoutputstream, unless you want the data thrown away?

Comment 13

[[e-mail]]

+     * to |write| succeeded and the stream was successfully closed. If the
+     * stream is closed by calling |close| directly, or the stream goes away,
+     * the original file will not be overwritten, and the temporary file will
+     * be deleted.

that's what's in the patch at the moment... is that okay?

and about the rv code... hmm, we don't really know that it's an OS error. i like
the NS_ERROR_LOSS... one though. darin, any objections?

Comment 14

[[e-mail]]

hmm, maybe i'll shift that comment sentence into the comment block before the
iface definition, it might belong better there.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #13)
> that's what's in the patch at the moment... is that okay?

oh, sorry, missed that. but yeah, I do think it fits better in the comment
before the interface

Comment 16

[[e-mail]]

Attachment: v2 (checked in)

this is what i checked in (and what should go on the aviary1.0 branch).