Re: svn commit: r1680819 - /subversion/trunk/subversion/libsvn_fs_fs/revprops.c

Philip Martin <philip.martin_at_wandisco.com> writes:

> Evgeny Kotkov <evgeny.kotkov_at_visualsvn.com> writes:
>
>> Or is it just a gut feeling that we should be using streams here?
>
> We have been gradually moving our file-based code to stream-based code
> for years.

I believe that flushing files (instead of achieving the same behavior with
streams) is a better choice in this particular context. The relevant bits of
code in libsvn_fs_fs/revprops.c that are responsible for persisting revision
property changes work with files, and we cannot do something with that. We
can abstract certain parts of this logic with streams — e.g., when it comes
to functions that serialize changes, because they do not really care if the
destination is a file or a generic stream.

However, an attempt to provide a proper abstraction for fsync() with streams
is a step towards dynamic typing with a possibility of a runtime error like
SVN_ERR_STREAM_NOT_SUPPORTED. I think that in this particular case using
apr_file_t is better than forcing the usage of svn_stream_t just because we
have been moving from files toward streams. We have a temporary file on the
disk, and we need to fsync() it when we finished working with it. What could
be more straightforward than calling svn_io_file_flush_to_disk() on the file
right before svn_io_file_close()?

I'd like to point out that we have an option of achieving the same behavior
with streams, if we're aiming towards this. While this is probably a slight
overkill, we could add svn_stream_flush() — inspired by design of the .NET
system library [1,2] — to our API and provide the necessary flush handlers.
This could be useful not just for files, but also for svn_stream_compressed()
and maybe for flushing network buffers if we ever need to do so.

I sketched this option in the attached patch. With this patch applied, we
could rework the r1680819 fix like below. What do you think?
[[[
Index: subversion/libsvn_fs_fs/revprops.c
===================================================================
--- subversion/libsvn_fs_fs/revprops.c (revision 1680705)
+++ subversion/libsvn_fs_fs/revprops.c (working copy)
@@ -661,6 +661,7 @@ write_non_packed_revprop(const char **final_path,
                                  svn_dirent_dirname(*final_path, pool),
                                  svn_io_file_del_none, pool, pool));
   SVN_ERR(svn_hash_write2(proplist, stream, SVN_HASH_TERMINATOR, pool));
+ SVN_ERR(svn_stream_flush(stream, TRUE));
   SVN_ERR(svn_stream_close(stream));

   return SVN_NO_ERROR;
@@ -811,9 +812,8 @@ repack_revprops(svn_fs_t *fs,
                           ? SVN_DELTA_COMPRESSION_LEVEL_DEFAULT
                           : SVN_DELTA_COMPRESSION_LEVEL_NONE));

- /* finally, write the content to the target stream and close it */
+ /* finally, write the content to the target stream */
   SVN_ERR(svn_stream_write(file_stream, compressed->data, &compressed->len));
- SVN_ERR(svn_stream_close(file_stream));

   return SVN_NO_ERROR;
 }
@@ -933,6 +933,8 @@ write_packed_revprop(const char **final_path,
       SVN_ERR(repack_revprops(fs, revprops, 0, revprops->sizes->nelts,
                               changed_index, serialized, new_total_size,
                               stream, pool));
+ SVN_ERR(svn_stream_flush(stream, TRUE));
+ SVN_ERR(svn_stream_close(stream));
     }
   else
     {
@@ -983,6 +985,8 @@ write_packed_revprop(const char **final_path,
           SVN_ERR(repack_revprops(fs, revprops, 0, left_count,
                                   changed_index, serialized, new_total_size,
                                   stream, pool));
+ SVN_ERR(svn_stream_flush(stream, TRUE));
+ SVN_ERR(svn_stream_close(stream));
         }

       if (left_count + right_count < revprops->sizes->nelts)
@@ -994,6 +998,8 @@ write_packed_revprop(const char **final_path,
                                   changed_index + 1,
                                   changed_index, serialized, new_total_size,
                                   stream, pool));
+ SVN_ERR(svn_stream_flush(stream, TRUE));
+ SVN_ERR(svn_stream_close(stream));
         }

       if (right_count)
@@ -1007,6 +1013,8 @@ write_packed_revprop(const char **final_path,
                                   revprops->sizes->nelts, changed_index,
                                   serialized, new_total_size, stream,
                                   pool));
+ SVN_ERR(svn_stream_flush(stream, TRUE));
+ SVN_ERR(svn_stream_close(stream));
         }

       /* write the new manifest */
@@ -1021,6 +1029,7 @@ write_packed_revprop(const char **final_path,
           SVN_ERR(svn_stream_printf(stream, pool, "%s\n", filename));
         }

+ SVN_ERR(svn_stream_flush(stream, TRUE));
       SVN_ERR(svn_stream_close(stream));
     }
]]]

[1] https://msdn.microsoft.com/en-us/library/system.io.stream.flush
[2] https://msdn.microsoft.com/en-us/library/ee474552

Regards,
Evgeny Kotkov