You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Erik Huelsmann <eh...@gmail.com> on 2005/12/11 21:13:22 UTC
Stacked resources (streams, files) && resource ownership
Whlie trying to optimize svn_wc_transmit_text_deltas, I was
implementing a function which is supposed to return a stream
(svn_wc_translated_stream()). Internally this stream will read from a
file, but this is an implementation detail (more on that later). The
problem is that I have a stack of streams and a file:
stream_translated
stream_from_aprfile
apr_file
All functions we have until now, keep references to all streams and
the file and close them as necessary.
On the other hand, we seem to have a 'close' mechanism on all kinds of
handlers (window handlers especially), by sending a final NULL window.
This NULL window is passed through to all handlers in the stack,
making this effectively the model where each handler is owned by the
handler stacked on top of it.
When looking at it this way, I think we have 2 different models of
resource ownership in the tree. I also think the model used for
stacked files and streams is slightly conflicting with pool
programming: the creator function needs to manually track all
resources stacked together to make sure they're all closed...
What I want to do is add streams which close the streams which they're
stacked upon and add a svn_stream_from_aprfile2() which takes a
close_file boolean to close the file when the stream is closed. That
way, we would grow to a model where all stacked resources own the
resource(s) they're stacked upon.
I shortly talked with Greg Hudson on irc about this, but since he had
to leave, I'm still looking for opinions...
bye,
Erik.
Re: Stacked resources (streams, files) && resource ownership
Posted by Erik Huelsmann <eh...@gmail.com>.
On 12/12/05, Greg Hudson <gh...@mit.edu> wrote:
> On Mon, 2005-12-12 at 23:03 +0100, Erik Huelsmann wrote:
> > Well, I could create an svn_stream_from_aprfile2 which conforms to the
> > above model. Whether svn_stream_from_aprfile should be immediately
> > deprecated is not clear to me, but we might as well.
On my quest through our sources to replace svn_stream_from_aprfile
with the ...2() version, I came across svn_client_cat2(). I decided to
replace the current implementation which uses temp files with one
using the new translating stream. It's a nice test case for th
desired ownership model.
The patch below works great, but there's a big problem with the ownership model:
it creates a stream to be stacked upon the 'out' stream (a parameter
passed iinto the routine). Clearly, we're given a stream which we
shouldn't close because the caller owns it. The stacked stream -
according to the model - will own the 'out' stream, but this conflicts
with the fact that it's actually a borrowed resource....
Apart from not changing the implementation, I hit a wall pretty fast.
Any ideas about solving this one? I thought of adding an explicitly
ownership-detaching stream which just forwards reads and writes to a
stream which it *doesn't* close upon being closed....
bye,
Erik.
Index: subversion/libsvn_client/cat.c
===================================================================
--- subversion/libsvn_client/cat.c (revision 17766)
+++ subversion/libsvn_client/cat.c (working copy)
@@ -144,7 +144,7 @@
SVN_ERR (svn_io_file_open (&input_file, base,
APR_READ, APR_OS_DEFAULT, pool));
- input = svn_stream_from_aprfile (input_file, pool);
+ input = svn_stream_from_aprfile2 (input_file, pool);
if ( eol || kw )
SVN_ERR (svn_subst_translate_stream3 (input, output, eol, FALSE, kw,
@@ -153,7 +153,6 @@
SVN_ERR (svn_stream_copy (input, output, pool));
SVN_ERR (svn_stream_close (input));
- SVN_ERR (svn_io_file_close (input_file, pool));
return SVN_NO_ERROR;
}
@@ -217,41 +216,19 @@
if (! eol_style && ! keywords)
{
/* It's a file with no special eol style or keywords. */
- SVN_ERR (svn_ra_get_file (ra_session, "", rev, out, NULL, NULL, pool));
- }
- else
- {
- apr_hash_t *kw = NULL;
- svn_subst_eol_style_t style;
- const char *temp_dir;
- const char *tmp_filename;
- svn_stream_t *tmp_stream;
- apr_file_t *tmp_file;
- apr_status_t apr_err;
- apr_off_t off = 0;
- const char *eol = NULL;
+ svn_subst_eol_style_t eol;
+ const char *eol_str;
+ apr_hash_t *kw;
- /* grab a temporary file to write the target to. */
- SVN_ERR (svn_io_temp_dir (&temp_dir, pool));
- SVN_ERR (svn_io_open_unique_file2
- (&tmp_file, &tmp_filename,
- svn_path_join (temp_dir, "tmp", pool), ".tmp",
- svn_io_file_del_on_close, pool));
-
- tmp_stream = svn_stream_from_aprfile (tmp_file, pool);
-
- SVN_ERR (svn_ra_get_file (ra_session, "", rev, tmp_stream,
- NULL, NULL, pool));
-
- /* rewind our stream. */
- apr_err = apr_file_seek (tmp_file, APR_SET, &off);
- if (apr_err)
- return svn_error_wrap_apr (apr_err, _("Can't seek in '%s'"),
- svn_path_local_style (tmp_filename, pool));
-
if (eol_style)
- svn_subst_eol_style_from_value (&style, &eol, eol_style->data);
+ svn_subst_eol_style_from_value (&eol, &eol_str, eol_style->data);
+ else
+ {
+ eol = svn_subst_eol_style_none;
+ eol_str = NULL;
+ }
+
if (keywords)
{
svn_string_t *cmt_rev, *cmt_date, *cmt_author;
@@ -267,21 +244,22 @@
SVN_ERR (svn_time_from_cstring (&when, cmt_date->data, pool));
SVN_ERR (svn_subst_build_keywords2
- (&kw, keywords->data,
+ (&kw, keywords->data,
cmt_rev->data,
url,
when,
cmt_author ? cmt_author->data : NULL,
pool));
}
+ else
+ kw = NULL;
- SVN_ERR (svn_subst_translate_stream3 (tmp_stream, out, eol, FALSE, kw,
- TRUE, pool));
-
- SVN_ERR (svn_stream_close (tmp_stream));
- SVN_ERR (svn_io_file_close (tmp_file, pool));
+ /* Interject a translating stream */
+ out = svn_subst_stream_translated (out, eol_str, FALSE, kw, TRUE, pool);
}
+ SVN_ERR (svn_ra_get_file (ra_session, "", rev, out, NULL, NULL, pool));
+
return SVN_NO_ERROR;
}
Re: Stacked resources (streams, files) && resource ownership
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2005-12-12 at 23:03 +0100, Erik Huelsmann wrote:
> Well, I could create an svn_stream_from_aprfile2 which conforms to the
> above model. Whether svn_stream_from_aprfile should be immediately
> deprecated is not clear to me, but we might as well.
>
> Would it be a good idea to store the above in a text file (in notes/
> for example), for later retrieval?
Sounds sane enough.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Stacked resources (streams, files) && resource ownership
Posted by Erik Huelsmann <eh...@gmail.com>.
On 12/12/05, Greg Hudson <gh...@mit.edu> wrote:
> On consideration, here is where I would like us to be:
>
> * Streams which read from or write to an underlying object own that
> object, i.e. closing the stream closes the underlying object, if
> applicable.
>
> * The layer (function or data type) which created a stream is
> responsible for closing it, except when the above rule applies.
>
> * Window handlers are thought of as an odd kind of stream, and passing
> the final NULL window is considered closing the stream.
>
> If you think of apply_textdelta as creating a window handler, then I
> don't think we're too far off. svn_stream_from_aprfile isn't owning its
> subsidiary file, svn_txdelta_apply is erroneously taking responsibility
> for closing the window stream it is passed, and there may be some other
> deviations.
Well, I could create an svn_stream_from_aprfile2 which conforms to the
above model. Whether svn_stream_from_aprfile should be immediately
deprecated is not clear to me, but we might as well.
Would it be a good idea to store the above in a text file (in notes/
for example), for later retrieval?
bye,
Erik.
Re: Stacked resources (streams, files) && resource ownership
Posted by Greg Hudson <gh...@MIT.EDU>.
On consideration, here is where I would like us to be:
* Streams which read from or write to an underlying object own that
object, i.e. closing the stream closes the underlying object, if
applicable.
* The layer (function or data type) which created a stream is
responsible for closing it, except when the above rule applies.
* Window handlers are thought of as an odd kind of stream, and passing
the final NULL window is considered closing the stream.
If you think of apply_textdelta as creating a window handler, then I
don't think we're too far off. svn_stream_from_aprfile isn't owning its
subsidiary file, svn_txdelta_apply is erroneously taking responsibility
for closing the window stream it is passed, and there may be some other
deviations.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org