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 <e....@gmx.net> on 2003/12/17 16:27:19 UTC
Fix Win32 file handle leak in translate-test.exe?
I'm running make check now, but could someone verify that this fixes the
reported file handle leak?
Thanks!
bye,
Erik.
PS: Karl, even though "if (d)" looks the same as the one you commented
about; it isn't: in the old code d could not be null or it would have to be a
valid close parameter. In this code I use d != NULL as a flag that d needs
closing. I thought to use a separate flag for it at first, but there is no system I
know which returns NULL as a valid *FILE.
Index: subversion/libsvn_subr/subst.c
===================================================================
--- subversion/libsvn_subr/subst.c (revision 8017)
+++ subversion/libsvn_subr/subst.c (working copy)
@@ -751,6 +751,7 @@
goto error;
err = svn_io_file_close (d, pool);
+ d = NULL;
if (err)
goto error;
@@ -761,6 +762,9 @@
return SVN_NO_ERROR;
error:
+ if (d)
+ svn_error_clear (svn_io_file_close (d, pool));
+
svn_error_clear (svn_io_remove_file (dst_tmp, pool));
return err;
--
+++ GMX - die erste Adresse für Mail, Message, More +++
Neu: Preissenkung für MMS und FreeMMS! http://www.gmx.net
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by Erik Huelsmann <e....@gmx.net>.
> I'm running make check now, but could someone verify that this fixes the
> reported file handle leak?
make check ran successfully on Linux. I can't verify Win32 though.
bye,
Erik.
--
+++ GMX - die erste Adresse für Mail, Message, More +++
Neu: Preissenkung für MMS und FreeMMS! http://www.gmx.net
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by John Szakmeister <jo...@szakmeister.net>.
On Wednesday 17 December 2003 20:10, Branko Čibej wrote:
> [snip]
> >[[[
> >Fix a handle leak that caused translate-test to fail on Windows because
> >an open file cannot be removed there.
> >
> >* subversion/libsvn_subr/subst.c (svn_subst_copy_and_translate): Use a
> > subpool to open all files and streams and destroy it to make sure
> > that all file handles are closed before returning.
> >]]]
>
> All ra_local tests pass on Windows with this patch. I'm testing ra_dav
> and ra_svn right now. If these tests pass, too (and I don't see any
> reason why they wouldn't), I'd like to include this patch in 0.35.0. I
> think the fact that it fixes a test failure is reason enough, and it's
> also minor enough to be safe.
>
> Votes, please?
+1.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by Branko Čibej <br...@xbc.nu>.
kfogel@collab.net wrote:
>Branko Čibej <br...@xbc.nu> writes:
>
>
>>Good point. In the meantime I found (and fixed) a subtle memory usage
>>bug in my patch, *sigh*. I'll commit to trunk as soon as I've run some
>>tests.
>>
>>
>
>Might as well post here for more review, while waiting?...
>
>
>
It's r8048 now.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by Branko Čibej <br...@xbc.nu>.
kfogel@collab.net wrote:
>Branko Čibej <br...@xbc.nu> writes:
>
>
>>Good point. In the meantime I found (and fixed) a subtle memory usage
>>bug in my patch, *sigh*. I'll commit to trunk as soon as I've run some
>>tests.
>>
>>
>
>Might as well post here for more review, while waiting?...
>
>
>
Sure.
[[[
Fix a handle leak that caused translate-test to fail on Windows because
an open file cannot be removed there.
* subversion/libsvn_subr/subst.c (svn_subst_copy_and_translate): Use a
subpool to open all files and streams and destroy it to make sure
that all file handles are closed before returning.
]]]
Index: subversion/libsvn_subr/subst.c
===================================================================
--- subversion/libsvn_subr/subst.c (revision 8044)
+++ subversion/libsvn_subr/subst.c (working copy)
@@ -41,6 +41,7 @@
#include "svn_io.h"
#include "svn_hash.h"
#include "svn_subst.h"
+#include "svn_pools.h"
void
svn_subst_eol_style_from_value (svn_subst_eol_style_t *style,
@@ -707,33 +708,44 @@
svn_boolean_t expand,
apr_pool_t *pool)
{
- const char *dst_tmp;
+ const char *dst_tmp = NULL;
svn_stream_t *src_stream, *dst_stream;
apr_file_t *s = NULL, *d = NULL; /* init to null important for APR */
svn_error_t *err;
+ apr_pool_t *subpool;
/* The easy way out: no translation needed, just copy. */
if (! (eol_str || keywords))
return svn_io_copy_file (src, dst, FALSE, pool);
+ subpool = svn_pool_create (pool);
+
/* Open source file. */
- SVN_ERR (svn_io_file_open (&s, src, APR_READ | APR_BUFFERED,
- APR_OS_DEFAULT, pool));
+ err = svn_io_file_open (&s, src, APR_READ | APR_BUFFERED,
+ APR_OS_DEFAULT, subpool);
+ if (err)
+ goto error;
/* For atomicity, we translate to a tmp file and
then rename the tmp file over the real destination. */
- SVN_ERR (svn_io_open_unique_file (&d, &dst_tmp, dst,
- ".tmp", FALSE, pool));
+ err = svn_io_open_unique_file (&d, &dst_tmp, dst,
+ ".tmp", FALSE, subpool);
+ /* Move the file name to a more permanent pool. */
+ if (dst_tmp)
+ dst_tmp = apr_pstrdup(pool, dst_tmp);
+
+ if (err)
+ goto error;
+
/* Now convert our two open files into streams. */
- src_stream = svn_stream_from_aprfile (s, pool);
- dst_stream = svn_stream_from_aprfile (d, pool);
+ src_stream = svn_stream_from_aprfile (s, subpool);
+ dst_stream = svn_stream_from_aprfile (d, subpool);
/* Translate src stream into dst stream. */
err = svn_subst_translate_stream (src_stream, dst_stream,
eol_str, repair, keywords, expand);
-
if (err)
goto error;
@@ -746,23 +758,26 @@
if (err)
goto error;
- err = svn_io_file_close (s, pool);
+ err = svn_io_file_close (s, subpool);
if (err)
goto error;
- err = svn_io_file_close (d, pool);
+ err = svn_io_file_close (d, subpool);
if (err)
- goto error;
+ goto error;
/* Now that dst_tmp contains the translated data, do the atomic rename. */
- err = svn_io_file_rename (dst_tmp, dst, pool);
+ err = svn_io_file_rename (dst_tmp, dst, subpool);
if (err)
goto error;
+ svn_pool_destroy (subpool);
return SVN_NO_ERROR;
+
error:
- svn_error_clear (svn_io_remove_file (dst_tmp, pool));
-
+ svn_pool_destroy (subpool); /* Make sure all files are closed first. */
+ if (dst_tmp)
+ svn_error_clear (svn_io_remove_file (dst_tmp, pool));
return err;
}
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by kf...@collab.net.
Branko Čibej <br...@xbc.nu> writes:
> Good point. In the meantime I found (and fixed) a subtle memory usage
> bug in my patch, *sigh*. I'll commit to trunk as soon as I've run some
> tests.
Might as well post here for more review, while waiting?...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by Branko Čibej <br...@xbc.nu>.
kfogel@collab.net wrote:
>Branko Čibej <br...@xbc.nu> writes:
>
>
>>All ra_local tests pass on Windows with this patch. I'm testing ra_dav
>>and ra_svn right now. If these tests pass, too (and I don't see any
>>reason why they wouldn't), I'd like to include this patch in 0.35.0. I
>>think the fact that it fixes a test failure is reason enough, and it's
>>also minor enough to be safe.
>>
>>Votes, please?
>>
>>
>
>It hasn't even been committed on trunk yet, has it? (I don't see it.)
>
>Let's have it there, so people can easily review it before voting. I
>know you posted it before, and I'm 99% sure it's the one I'm quoting
>below, but it needs to go on trunk anyway, and once it's there, we
>have a concrete revision to talk about.
>
>
Good point. In the meantime I found (and fixed) a subtle memory usage
bug in my patch, *sigh*. I'll commit to trunk as soon as I've run some
tests.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by kf...@collab.net.
Branko Čibej <br...@xbc.nu> writes:
> All ra_local tests pass on Windows with this patch. I'm testing ra_dav
> and ra_svn right now. If these tests pass, too (and I don't see any
> reason why they wouldn't), I'd like to include this patch in 0.35.0. I
> think the fact that it fixes a test failure is reason enough, and it's
> also minor enough to be safe.
>
> Votes, please?
It hasn't even been committed on trunk yet, has it? (I don't see it.)
Let's have it there, so people can easily review it before voting. I
know you posted it before, and I'm 99% sure it's the one I'm quoting
below, but it needs to go on trunk anyway, and once it's there, we
have a concrete revision to talk about.
-K
Brane's patch:
[[[
Fix a handle leak that caused translate-test to fail on Windows because
an open file cannot be removed there.
* subversion/libsvn_subr/subst.c (svn_subst_copy_and_translate): Use a
subpool to open all files and streams and destroy it to make sure
that all file handles are closed before returning.
]]]
Index: subst.c
===================================================================
--- subst.c (revision 8027)
+++ subst.c (working copy)
@@ -41,6 +41,7 @@
#include "svn_io.h"
#include "svn_hash.h"
#include "svn_subst.h"
+#include "svn_pools.h"
void
svn_subst_eol_style_from_value (svn_subst_eol_style_t *style,
@@ -711,29 +712,35 @@
svn_stream_t *src_stream, *dst_stream;
apr_file_t *s = NULL, *d = NULL; /* init to null important for APR */
svn_error_t *err;
+ apr_pool_t *subpool;
/* The easy way out: no translation needed, just copy. */
if (! (eol_str || keywords))
return svn_io_copy_file (src, dst, FALSE, pool);
+ subpool = svn_pool_create (pool);
+
/* Open source file. */
- SVN_ERR (svn_io_file_open (&s, src, APR_READ | APR_BUFFERED,
- APR_OS_DEFAULT, pool));
+ err = svn_io_file_open (&s, src, APR_READ | APR_BUFFERED,
+ APR_OS_DEFAULT, subpool);
+ if (err)
+ goto error;
/* For atomicity, we translate to a tmp file and
then rename the tmp file over the real destination. */
- SVN_ERR (svn_io_open_unique_file (&d, &dst_tmp, dst,
- ".tmp", FALSE, pool));
+ err = svn_io_open_unique_file (&d, &dst_tmp, dst,
+ ".tmp", FALSE, subpool);
+ if (err)
+ goto error;
/* Now convert our two open files into streams. */
- src_stream = svn_stream_from_aprfile (s, pool);
- dst_stream = svn_stream_from_aprfile (d, pool);
+ src_stream = svn_stream_from_aprfile (s, subpool);
+ dst_stream = svn_stream_from_aprfile (d, subpool);
/* Translate src stream into dst stream. */
err = svn_subst_translate_stream (src_stream, dst_stream,
eol_str, repair, keywords, expand);
-
if (err)
goto error;
@@ -746,23 +753,25 @@
if (err)
goto error;
- err = svn_io_file_close (s, pool);
+ err = svn_io_file_close (s, subpool);
if (err)
goto error;
- err = svn_io_file_close (d, pool);
+ err = svn_io_file_close (d, subpool);
if (err)
- goto error;
+ goto error;
/* Now that dst_tmp contains the translated data, do the atomic rename. */
- err = svn_io_file_rename (dst_tmp, dst, pool);
+ err = svn_io_file_rename (dst_tmp, dst, subpool);
if (err)
goto error;
+ svn_pool_destroy (subpool);
return SVN_NO_ERROR;
+
error:
+ svn_pool_destroy (subpool); /* Make sure all files are closed first. */
svn_error_clear (svn_io_remove_file (dst_tmp, pool));
-
return err;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by Branko Čibej <br...@xbc.nu>.
Branko Čibej wrote:
>Branko Čibej wrote:
>
>
>
>>Branko Čibej wrote:
>>
>>
>>
>>
>>
>>>Erik Huelsmann wrote:
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>>I'm running make check now, but could someone verify that this fixes the
>>>>reported file handle leak?
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>This change of course doesn't plug the handle leak, because, there are a
>>>few "SVN_ERR" and "goto error" places before that could cause the file
>>>to remain open. I'm testing a different change right now.
>>>
>>>
>>>
>>>
>>>
>>>
>>There she is. With this change, translate-test passes on Windows. I'm
>>running the other tests now. This is all against HEAD of the 0.35.0
>>branch, by the way.
>>
>>[[[
>>Fix a handle leak that caused translate-test to fail on Windows because
>>an open file cannot be removed there.
>>
>>* subversion/libsvn_subr/subst.c (svn_subst_copy_and_translate): Use a
>> subpool to open all files and streams and destroy it to make sure
>> that all file handles are closed before returning.
>>]]]
>>
>>
>>
>>
>
>All ra_local tests pass on Windows with this patch. I'm testing ra_dav
>and ra_svn right now. If these tests pass, too (and I don't see any
>reason why they wouldn't), I'd like to include this patch in 0.35.0. I
>think the fact that it fixes a test failure is reason enough, and it's
>also minor enough to be safe.
>
>Votes, please?
>
>
All tests pass, BTW.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by Branko Čibej <br...@xbc.nu>.
Branko Čibej wrote:
>Branko Čibej wrote:
>
>
>
>>Erik Huelsmann wrote:
>>
>>
>>
>>
>>
>>>I'm running make check now, but could someone verify that this fixes the
>>>reported file handle leak?
>>>
>>>
>>>
>>>
>>>
>>>
>>This change of course doesn't plug the handle leak, because, there are a
>>few "SVN_ERR" and "goto error" places before that could cause the file
>>to remain open. I'm testing a different change right now.
>>
>>
>>
>>
>
>There she is. With this change, translate-test passes on Windows. I'm
>running the other tests now. This is all against HEAD of the 0.35.0
>branch, by the way.
>
>[[[
>Fix a handle leak that caused translate-test to fail on Windows because
>an open file cannot be removed there.
>
>* subversion/libsvn_subr/subst.c (svn_subst_copy_and_translate): Use a
> subpool to open all files and streams and destroy it to make sure
> that all file handles are closed before returning.
>]]]
>
>
All ra_local tests pass on Windows with this patch. I'm testing ra_dav
and ra_svn right now. If these tests pass, too (and I don't see any
reason why they wouldn't), I'd like to include this patch in 0.35.0. I
think the fact that it fixes a test failure is reason enough, and it's
also minor enough to be safe.
Votes, please?
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by Branko Čibej <br...@xbc.nu>.
Erik Huelsmann wrote:
>>>In the deltification-in-Apache case, it is both the only way and the
>>>documented way to do cleanup after sending a response to a client. I
>>>don't like it there either, but it was the best available option.
>>>That's not the case with file handles.
>>>
>>>
>>Exactly.
>>
>>
>I don't get it: you depend on pool cleanup for closing the files yourself.
>This contradicts what you have said until now. The only difference is that you
>introduced a new pool to guarantee the files have been cleaned up at a
>certain moment.
>
>
And that is the crucial difference. I never said pool cleanup shouldn't
be used to close file handles -- after all, apr_file_close is nothing
but a wrapper for pool cleanup. But we must control when files are closed.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by Erik Huelsmann <e....@gmx.net>.
> >In the deltification-in-Apache case, it is both the only way and the
> >documented way to do cleanup after sending a response to a client. I
> >don't like it there either, but it was the best available option.
> >That's not the case with file handles.
> Exactly.
I don't get it: you depend on pool cleanup for closing the files yourself.
This contradicts what you have said until now. The only difference is that you
introduced a new pool to guarantee the files have been cleaned up at a
certain moment.
> And I'm totally against "fixing" this bug by changing the test. My patch
> fixes the cause, and I vote we include it in 0.35.0.
I didn't mean we have to change the tests. Never intended to declare the
tests wrong even.
Anyway; I'll stop about it now.
The fix is one of many possible but since it works I'm +1 on the patch and
including it in 0.35.0.
bye,
Erik.
--
+++ GMX - die erste Adresse für Mail, Message, More +++
Neu: Preissenkung für MMS und FreeMMS! http://www.gmx.net
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by Branko Čibej <br...@xbc.nu>.
kfogel@collab.net wrote:
>"Erik Huelsmann" <e....@gmx.net> writes:
>
>
>>You mean that you think it *is* appropriate to depend on pool cleanup
>>for something complex as deltification, memory release and stuff like
>>that, but you don't consider it a good idea to use it for file
>>cleanup?!
>>
>>
>
>In the deltification-in-Apache case, it is both the only way and the
>documented way to do cleanup after sending a response to a client. I
>don't like it there either, but it was the best available option.
>
>That's not the case with file handles.
>
>
Exactly.
And I'm totally against "fixing" this bug by changing the test. My patch
fixes the cause, and I vote we include it in 0.35.0.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by kf...@collab.net.
"Erik Huelsmann" <e....@gmx.net> writes:
> You mean that you think it *is* appropriate to depend on pool cleanup
> for something complex as deltification, memory release and stuff like
> that, but you don't consider it a good idea to use it for file
> cleanup?!
In the deltification-in-Apache case, it is both the only way and the
documented way to do cleanup after sending a response to a client. I
don't like it there either, but it was the best available option.
That's not the case with file handles.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by Erik Huelsmann <e....@gmx.net>.
> Greg Hudson <gh...@MIT.EDU> writes:
> > Can you better explain how translate-test was failing? Were we running
> > the translate command in a loop without cleaning a pool?
> >
> > I continue to believe that it's appropriate to rely on pool cleanups for
>
> Did you mean "inappropriate"?
>
> (Serious question.)
You mean that you think it *is* appropriate to depend on pool cleanup for
something complex as deltification, memory release and stuff like that, but you
don't consider it a good idea to use it for file cleanup?!
We might need a different cleaup function registered to accomplish what we
want, but I must say I agree with Greg if he meant "approprioate".
bye,
Erik.
--
+++ GMX - die erste Adresse für Mail, Message, More +++
Neu: Preissenkung für MMS und FreeMMS! http://www.gmx.net
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by Greg Hudson <gh...@MIT.EDU>.
On Thu, 2003-12-18 at 05:58, Erik Huelsmann wrote:
> > No, I spoke correctly.
> >
> > Though perhaps unclearly: by "it's a bug either way," I mean if you are
> > running an unbounded loop without pool cleanups, you have a bug. If you
> > notice this bug because a file handle doesn't get cleaned up, then
> > that's your fault, not the fault of the function which left a file
> > handle to be closed by pool cleanup.
>
> Does this mean you don't like what Brane did to the function with his patch?
> There is / was a bug in the function itself: the function did not close a
> file when trying to remove it. If this causes a handle leak on Windows that is
> a problem which should be addressed. Branes patch does that.
I misunderstood the problem. I think brane's fix is fine.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by Erik Huelsmann <e....@gmx.net>.
> No, I spoke correctly.
>
> Though perhaps unclearly: by "it's a bug either way," I mean if you are
> running an unbounded loop without pool cleanups, you have a bug. If you
> notice this bug because a file handle doesn't get cleaned up, then
> that's your fault, not the fault of the function which left a file
> handle to be closed by pool cleanup.
Does this mean you don't like what Brane did to the function with his patch?
There is / was a bug in the function itself: the function did not close a
file when trying to remove it. If this causes a handle leak on Windows that is
a problem which should be addressed. Branes patch does that.
Brane told me I was doing something dumb (removing before closing), although
I don't get why we had a leak since the APR docs say:
/**
* delete the specified file.
* @param path The full path to the file (using / on all systems)
* @param cont The pool to use.
* @remark If the file is open, it won't be removed until all instances are
closed.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
*/
APR_DECLARE(apr_status_t) apr_file_remove(const char *path, apr_pool_t
*cont);
If there are better solutions, maybe we should postpone futher discussion
until after 1.0 if Branes patch works? The idea on which I have based the patch
I showed you in IRC yesterday is in my opinion a more elegant way of
addressing the problem, but I seem to have gotten on various nerves already and
discussion could be done later.
bye,
Erik.
PS: The patch can be retrieved at
http://encorps.dnsalias.com/svn-public/patches/svn_subst_copy_and_translate-cleanup.patch and probably needs cleanup
before it can be applied. It's the idea that counts.
--
+++ GMX - die erste Adresse für Mail, Message, More +++
Neu: Preissenkung für MMS und FreeMMS! http://www.gmx.net
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by kf...@collab.net.
Greg Hudson <gh...@MIT.EDU> writes:
> Though perhaps unclearly: by "it's a bug either way," I mean if you are
> running an unbounded loop without pool cleanups, you have a bug. If you
> notice this bug because a file handle doesn't get cleaned up, then
> that's your fault, not the fault of the function which left a file
> handle to be closed by pool cleanup.
Amen!
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by Greg Hudson <gh...@MIT.EDU>.
On Wed, 2003-12-17 at 15:32, kfogel@collab.net wrote:
> Greg Hudson <gh...@MIT.EDU> writes:
> > Can you better explain how translate-test was failing? Were we running
> > the translate command in a loop without cleaning a pool?
> >
> > I continue to believe that it's appropriate to rely on pool cleanups for
>
> Did you mean "inappropriate"?
No, I spoke correctly.
Though perhaps unclearly: by "it's a bug either way," I mean if you are
running an unbounded loop without pool cleanups, you have a bug. If you
notice this bug because a file handle doesn't get cleaned up, then
that's your fault, not the fault of the function which left a file
handle to be closed by pool cleanup.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by kf...@collab.net.
Greg Hudson <gh...@MIT.EDU> writes:
> Can you better explain how translate-test was failing? Were we running
> the translate command in a loop without cleaning a pool?
>
> I continue to believe that it's appropriate to rely on pool cleanups for
Did you mean "inappropriate"?
(Serious question.)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by Branko Čibej <br...@xbc.nu>.
Greg Hudson wrote:
>Can you better explain how translate-test was failing? Were we running
>the translate command in a loop without cleaning a pool?
>
>I continue to believe that it's appropriate to rely on pool cleanups for
>files as well as memory. It may take a smaller loop without pool
>cleanup to notice a resource problem with open files, but it's a bug
>either way.
>
>
The problem is that the bug was _within_ the function itself, regardless
of pool cleanup -- in the case of an error, it attempted to remove the
temporary file even though it had not been closed first. That has
nothing to do with the concept of pool cleanup for closing files.
My patch fixes this bug by using pool cleanup to make sure the files get
closed.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by Greg Hudson <gh...@MIT.EDU>.
Can you better explain how translate-test was failing? Were we running
the translate command in a loop without cleaning a pool?
I continue to believe that it's appropriate to rely on pool cleanups for
files as well as memory. It may take a smaller loop without pool
cleanup to notice a resource problem with open files, but it's a bug
either way.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by Branko Čibej <br...@xbc.nu>.
Branko Čibej wrote:
>Erik Huelsmann wrote:
>
>
>
>>I'm running make check now, but could someone verify that this fixes the
>>reported file handle leak?
>>
>>
>>
>>
>This change of course doesn't plug the handle leak, because, there are a
>few "SVN_ERR" and "goto error" places before that could cause the file
>to remain open. I'm testing a different change right now.
>
>
There she is. With this change, translate-test passes on Windows. I'm
running the other tests now. This is all against HEAD of the 0.35.0
branch, by the way.
[[[
Fix a handle leak that caused translate-test to fail on Windows because
an open file cannot be removed there.
* subversion/libsvn_subr/subst.c (svn_subst_copy_and_translate): Use a
subpool to open all files and streams and destroy it to make sure
that all file handles are closed before returning.
]]]
Index: subst.c
===================================================================
--- subst.c (revision 8027)
+++ subst.c (working copy)
@@ -41,6 +41,7 @@
#include "svn_io.h"
#include "svn_hash.h"
#include "svn_subst.h"
+#include "svn_pools.h"
void
svn_subst_eol_style_from_value (svn_subst_eol_style_t *style,
@@ -711,29 +712,35 @@
svn_stream_t *src_stream, *dst_stream;
apr_file_t *s = NULL, *d = NULL; /* init to null important for APR */
svn_error_t *err;
+ apr_pool_t *subpool;
/* The easy way out: no translation needed, just copy. */
if (! (eol_str || keywords))
return svn_io_copy_file (src, dst, FALSE, pool);
+ subpool = svn_pool_create (pool);
+
/* Open source file. */
- SVN_ERR (svn_io_file_open (&s, src, APR_READ | APR_BUFFERED,
- APR_OS_DEFAULT, pool));
+ err = svn_io_file_open (&s, src, APR_READ | APR_BUFFERED,
+ APR_OS_DEFAULT, subpool);
+ if (err)
+ goto error;
/* For atomicity, we translate to a tmp file and
then rename the tmp file over the real destination. */
- SVN_ERR (svn_io_open_unique_file (&d, &dst_tmp, dst,
- ".tmp", FALSE, pool));
+ err = svn_io_open_unique_file (&d, &dst_tmp, dst,
+ ".tmp", FALSE, subpool);
+ if (err)
+ goto error;
/* Now convert our two open files into streams. */
- src_stream = svn_stream_from_aprfile (s, pool);
- dst_stream = svn_stream_from_aprfile (d, pool);
+ src_stream = svn_stream_from_aprfile (s, subpool);
+ dst_stream = svn_stream_from_aprfile (d, subpool);
/* Translate src stream into dst stream. */
err = svn_subst_translate_stream (src_stream, dst_stream,
eol_str, repair, keywords, expand);
-
if (err)
goto error;
@@ -746,23 +753,25 @@
if (err)
goto error;
- err = svn_io_file_close (s, pool);
+ err = svn_io_file_close (s, subpool);
if (err)
goto error;
- err = svn_io_file_close (d, pool);
+ err = svn_io_file_close (d, subpool);
if (err)
- goto error;
+ goto error;
/* Now that dst_tmp contains the translated data, do the atomic rename. */
- err = svn_io_file_rename (dst_tmp, dst, pool);
+ err = svn_io_file_rename (dst_tmp, dst, subpool);
if (err)
goto error;
+ svn_pool_destroy (subpool);
return SVN_NO_ERROR;
+
error:
+ svn_pool_destroy (subpool); /* Make sure all files are closed first. */
svn_error_clear (svn_io_remove_file (dst_tmp, pool));
-
return err;
}
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Fix Win32 file handle leak in translate-test.exe?
Posted by Branko Čibej <br...@xbc.nu>.
Erik Huelsmann wrote:
>I'm running make check now, but could someone verify that this fixes the
>reported file handle leak?
>
>
This change of course doesn't plug the handle leak, because, there are a
few "SVN_ERR" and "goto error" places before that could cause the file
to remain open. I'm testing a different change right now.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org