You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Hudson <gh...@MIT.EDU> on 2003/10/10 19:45:38 UTC
Re: Detecting unhandled errors, WAS: RE: svn commit: rev 7373 -
trunk/subversion/libsvn_client
On Fri, 2003-10-10 at 15:24, Sander Striker wrote:
> My vote is for:
>
> static apr_status_t err_abort(void *data)
> {
> abort();
> }
A couple of remarks:
* We should expect this to introduce a period of instability into our
client when we first introduce it. Perhaps it should be limited to
enable-maintainer-mode builds at least for a while.
* If we ever enable it for non-maintainer-mode builds, perhaps spewing
to stderr and exiting would be better than an abort() for those builds.
* This will catch errors dropped by applications as well as errors we
drop in our own libraries. (At least, if the applications perform an
APR shutdown.) That might be considered a little unfriendly; there are
many cases where an application doesn't need to care about a small
amount of leaked memory, particularly when it only happens a constant
number of times.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: Detecting unhandled errors
Posted by Sander Striker <st...@apache.org>.
> From: Philip Martin [mailto:philip@codematters.co.uk]
> Sent: Monday, October 13, 2003 3:52 PM
> Did you run the right test number in gdb? The error is leaked in test
> 7, but the abort happens at the end because that's when the pool is
> cleared.
Yes. I ran the entire test to be sure.
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Detecting unhandled errors
Posted by Philip Martin <ph...@codematters.co.uk>.
"Sander Striker" <st...@apache.org> writes:
> Running all tests in run-fs-tests.py...FAILURE
>
> I didn't get a core file, nor did gdb help me out when running
> the test in gdb. Running outside gdb however:
>
> PASS: lt-fs-test 37: ensure no-op for redundant copies
> Aborted
>
> Clues?
Did you run the right test number in gdb? The error is leaked in test
7, but the abort happens at the end because that's when the pool is
cleared.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: Detecting unhandled errors
Posted by Sander Striker <st...@apache.org>.
> From: Philip Martin [mailto:philip@codematters.co.uk]
> Sent: Monday, October 13, 2003 5:44 PM
> "C. Michael Pilato" <cm...@collab.net> writes:
>
> > "Sander Striker" <st...@apache.org> writes:
> >
> >> Ah, right!
> >>
> >> So, would this do the trick? Is this desired, or should the caller clear
> >> the error itself?
> >
> > Feels (to me) more correct for the consumers to explicitly clear the
> > errors they are finished with.
>
> Not sure which is correct, after all the function does have "handle"
> in it's name.
I agree.
> I think I have identified all the fs-test error leaks, they are all in
> fs-test.c itself.
Great, thx.
> One thing I noticed is that when an program creates
> and clears lots of errors it can be very hard to determine which error
> got missed. I am using a local patch to register the error itself as
> the cleanup data pointer. What do you think?
All goodness, +1.
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Detecting unhandled errors
Posted by Philip Martin <ph...@codematters.co.uk>.
"C. Michael Pilato" <cm...@collab.net> writes:
> "Sander Striker" <st...@apache.org> writes:
>
>> Ah, right!
>>
>> So, would this do the trick? Is this desired, or should the caller clear
>> the error itself?
>
> Feels (to me) more correct for the consumers to explicitly clear the
> errors they are finished with.
Not sure which is correct, after all the function does have "handle"
in it's name. If we do clear the error then it should be documented
in svn_error.h.
I think I have identified all the fs-test error leaks, they are all in
fs-test.c itself. One thing I noticed is that when an program creates
and clears lots of errors it can be very hard to determine which error
got missed. I am using a local patch to register the error itself as
the cleanup data pointer. What do you think?
Index: subversion/libsvn_subr/error.c
===================================================================
--- subversion/libsvn_subr/error.c (revision 7398)
+++ subversion/libsvn_subr/error.c (working copy)
@@ -66,7 +66,9 @@
#if defined(SVN_DEBUG_ERROR)
static apr_status_t err_abort (void *data)
{
+ svn_error_t *err = data;
abort();
+ err = NULL;
}
#endif
@@ -85,9 +87,6 @@
{
if (apr_pool_create (&pool, NULL))
abort ();
-#if defined(SVN_DEBUG_ERROR)
- apr_pool_cleanup_register(pool, NULL, err_abort, NULL);
-#endif
}
/* Create the new error structure */
@@ -101,6 +100,11 @@
new_error->line = error_line;
/* XXX TODO: Unlock mutex here */
+#if defined(SVN_DEBUG_ERROR)
+ if (! child)
+ apr_pool_cleanup_register(pool, new_error, err_abort, NULL);
+#endif
+
return new_error;
}
@@ -161,6 +165,10 @@
while (chain->child)
chain = chain->child;
+#if defined(SVN_DEBUG_ERROR)
+ apr_pool_cleanup_kill (pool, chain, err_abort);
+#endif
+
/* Copy the new error chain into the old chain's pool. */
while (new_err)
{
@@ -169,13 +177,18 @@
*chain = *new_err;
chain->message = apr_pstrdup (pool, new_err->message);
chain->pool = pool;
+#if defined(SVN_DEBUG_ERROR)
+ if (! new_err->child)
+ apr_pool_cleanup_kill (oldpool, new_err, err_abort);
+#endif
new_err = new_err->child;
}
- /* Destroy the new error chain. */
#if defined(SVN_DEBUG_ERROR)
- apr_pool_cleanup_kill (oldpool, NULL, err_abort);
+ apr_pool_cleanup_register (pool, chain, err_abort, NULL);
#endif
+
+ /* Destroy the new error chain. */
apr_pool_destroy (oldpool);
}
@@ -186,7 +199,9 @@
if (err)
{
#if defined(SVN_DEBUG_ERROR)
- apr_pool_cleanup_kill (err->pool, NULL, err_abort);
+ while (err->child)
+ err = err->child;
+ apr_pool_cleanup_kill (err->pool, err, err_abort);
#endif
apr_pool_destroy (err->pool);
}
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Detecting unhandled errors
Posted by "C. Michael Pilato" <cm...@collab.net>.
"Sander Striker" <st...@apache.org> writes:
> Ah, right!
>
> So, would this do the trick? Is this desired, or should the caller clear
> the error itself?
Feels (to me) more correct for the consumers to explicitly clear the
errors they are finished with.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: Detecting unhandled errors
Posted by Sander Striker <st...@apache.org>.
> From: Philip Martin [mailto:philip@codematters.co.uk]
> Sent: Monday, October 13, 2003 4:47 PM
[...]
> svn_handle_error doesn't do the right thing
>
> $ svn ls
> ../svn/subversion/libsvn_wc/lock.c:494: (apr_err=155007)
> svn: Path is not a working copy directory
> svn: '.' is not a working copy
> Aborted
>
> I think this is the "feature" that the regression tests don't detect a
> client that aborts. We could do it on Unix but not on Windows, and we
> don't want any tests to rely on platform-specific error detection.
Ah, right!
So, would this do the trick? Is this desired, or should the caller clear
the error itself?
Sander
Log:
Clear handled errors.
* subversion/libsvn_subr/error.c
(svn_handle_error, svn_handle_warning): Clear error after consumption.
Index: subversion/libsvn_subr/error.c
===================================================================
--- subversion/libsvn_subr/error.c (revision 7398)
+++ subversion/libsvn_subr/error.c (working copy)
@@ -261,6 +261,8 @@
svn_handle_error (svn_error_t *err, FILE *stream, svn_boolean_t fatal)
{
handle_error (err, stream, fatal, 0, APR_SUCCESS);
+
+ svn_error_clear (err);
}
@@ -270,6 +272,8 @@
fprintf (stream, "svn: warning: %s\n",
convert_string_for_output (err->message, err->pool));
fflush (stream);
+
+ svn_error_clear (err);
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Detecting unhandled errors
Posted by Philip Martin <ph...@codematters.co.uk>.
"Sander Striker" <st...@apache.org> writes:
>> From: cmpilato@localhost.localdomain
>> [mailto:cmpilato@localhost.localdomain]On Behalf Of C. Michael Pilato
>> Sent: Monday, October 13, 2003 3:42 PM
>
> [...]
>> Well, I fully expected things to fail with only the patch I sent. We
>> still don't have any code cleaning up errors we *do* handle. That is,
>> the client should be clearing the error chain after it calls
>> svn_handle_error() (or perhaps svn_handle_error() should remove the
>> deathly pool cleanup functions).
>
> Well, actually, all the cmdline client tests succeed. So apparently it
> isn't that bad.
svn_handle_error doesn't do the right thing
$ svn ls
../svn/subversion/libsvn_wc/lock.c:494: (apr_err=155007)
svn: Path is not a working copy directory
svn: '.' is not a working copy
Aborted
I think this is the "feature" that the regression tests don't detect a
client that aborts. We could do it on Unix but not on Windows, and we
don't want any tests to rely on platform-specific error detection.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: Detecting unhandled errors
Posted by Sander Striker <st...@apache.org>.
> From: cmpilato@localhost.localdomain
> [mailto:cmpilato@localhost.localdomain]On Behalf Of C. Michael Pilato
> Sent: Monday, October 13, 2003 3:42 PM
[...]
> Well, I fully expected things to fail with only the patch I sent. We
> still don't have any code cleaning up errors we *do* handle. That is,
> the client should be clearing the error chain after it calls
> svn_handle_error() (or perhaps svn_handle_error() should remove the
> deathly pool cleanup functions).
Well, actually, all the cmdline client tests succeed. So apparently it
isn't that bad.
/me wonders if we are calling apr_terminate()...
Hmmm, we are, unless atexit isn't working properly, or we are bypassing
in some way.
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Detecting unhandled errors
Posted by "C. Michael Pilato" <cm...@collab.net>.
"Sander Striker" <st...@apache.org> writes:
> +1. I've committed this in rev 7398, with #if ... #endif wrappers.
>
> 2 tests are failing with this turned on.
>
> Running all tests in translate-test...FAILURE
> Running all tests in run-fs-tests.py...FAILURE
>
> I didn't get a core file, nor did gdb help me out when running
> the test in gdb. Running outside gdb however:
>
> PASS: lt-fs-test 37: ensure no-op for redundant copies
> Aborted
>
> Clues?
Well, I fully expected things to fail with only the patch I sent. We
still don't have any code cleaning up errors we *do* handle. That is,
the client should be clearing the error chain after it calls
svn_handle_error() (or perhaps svn_handle_error() should remove the
deathly pool cleanup functions).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: Detecting unhandled errors
Posted by Sander Striker <st...@apache.org>.
> From: cmpilato@localhost.localdomain
> [mailto:cmpilato@localhost.localdomain]On Behalf Of C. Michael Pilato
> Sent: Saturday, October 11, 2003 3:28 PM
[...]
>> That's true. So maybe it needs to be wrapped in another #if block;
>> SVN_DEBUG_ERROR_LEAK, or something alike?
>
> This is exactly what I was about to do (except, using
> SVN_ERROR_DEBUG). But I don't have time to code or test anything
> right now, so here's the patch (obviously sans-#ifdefs). It's just
> like what Sander sent, except it also removes the aborts before try to
> destroy a child error's pool when that error has been duped into
> another pool.
+1. I've committed this in rev 7398, with #if ... #endif wrappers.
2 tests are failing with this turned on.
Running all tests in translate-test...FAILURE
Running all tests in run-fs-tests.py...FAILURE
I didn't get a core file, nor did gdb help me out when running
the test in gdb. Running outside gdb however:
PASS: lt-fs-test 37: ensure no-op for redundant copies
Aborted
Clues?
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Detecting unhandled errors
Posted by "C. Michael Pilato" <cm...@collab.net>.
"Sander Striker" <st...@apache.org> writes:
> > > * This will catch errors dropped by applications as well as errors we
> > > drop in our own libraries. (At least, if the applications perform an
> > > APR shutdown.) That might be considered a little unfriendly; there are
> > > many cases where an application doesn't need to care about a small
> > > amount of leaked memory, particularly when it only happens a constant
> > > number of times.
>
> That's true. So maybe it needs to be wrapped in another #if block;
> SVN_DEBUG_ERROR_LEAK, or something alike?
This is exactly what I was about to do (except, using
SVN_ERROR_DEBUG). But I don't have time to code or test anything
right now, so here's the patch (obviously sans-#ifdefs). It's just
like what Sander sent, except it also removes the aborts before try to
destroy a child error's pool when that error has been duped into
another pool.
--
* subversion/libsvn_subr/error.c
(err_abort): New.
(make_error_internal): Register err_abort() as a cleanup routine for
new error pools.
(svn_error_clear, svn_error_compose): Remove err_abort() from the
cleanup routine list on the error's pool.
Index: subversion/libsvn_subr/error.c
===================================================================
--- subversion/libsvn_subr/error.c (revision 7373)
+++ subversion/libsvn_subr/error.c (working copy)
@@ -61,6 +61,14 @@
}
+/* Cleanup function for errors. svn_error_clear () removes this so
+ errors that are properly handled *don't* hit this code. */
+static apr_status_t err_abort (void *data)
+{
+ abort();
+}
+
+
static svn_error_t *
make_error_internal (apr_status_t apr_err,
svn_error_t *child)
@@ -71,8 +79,12 @@
/* Reuse the child's pool, or create our own. */
if (child)
pool = child->pool;
- else if (apr_pool_create (&pool, NULL))
- abort ();
+ else
+ {
+ if (apr_pool_create (&pool, NULL))
+ abort ();
+ apr_pool_cleanup_register(pool, NULL, err_abort, NULL);
+ }
/* Create the new error structure */
new_error = (svn_error_t *) apr_pcalloc (pool, sizeof (*new_error));
@@ -157,6 +169,7 @@
}
/* Destroy the new error chain. */
+ apr_pool_cleanup_kill (oldpool, NULL, err_abort);
apr_pool_destroy (oldpool);
}
@@ -165,7 +178,10 @@
svn_error_clear (svn_error_t *err)
{
if (err)
- apr_pool_destroy (err->pool);
+ {
+ apr_pool_cleanup_kill (err->pool, NULL, err_abort);
+ apr_pool_destroy (err->pool);
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: Detecting unhandled errors
Posted by Sander Striker <st...@apache.org>.
> From: cmpilato@localhost.localdomain
> [mailto:cmpilato@localhost.localdomain]On Behalf Of C. Michael Pilato
> Sent: Friday, October 10, 2003 9:53 PM
> Greg Hudson <gh...@MIT.EDU> writes:
[...]
> > * We should expect this to introduce a period of instability into our
> > client when we first introduce it. Perhaps it should be limited to
> > enable-maintainer-mode builds at least for a while.
It should be enabled only for maintainer mode.
> > * If we ever enable it for non-maintainer-mode builds, perhaps spewing
> > to stderr and exiting would be better than an abort() for those builds.
I'd like to never enable it for non-maintainer-mode.
> > * This will catch errors dropped by applications as well as errors we
> > drop in our own libraries. (At least, if the applications perform an
> > APR shutdown.) That might be considered a little unfriendly; there are
> > many cases where an application doesn't need to care about a small
> > amount of leaked memory, particularly when it only happens a constant
> > number of times.
That's true. So maybe it needs to be wrapped in another #if block;
SVN_DEBUG_ERROR_LEAK, or something alike?
> Hmm... good points, all. Must think on these things.
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Detecting unhandled errors, WAS: RE: svn commit: rev 7373 - trunk/subversion/libsvn_client
Posted by "C. Michael Pilato" <cm...@collab.net>.
Greg Hudson <gh...@MIT.EDU> writes:
> On Fri, 2003-10-10 at 15:24, Sander Striker wrote:
> > My vote is for:
> >
> > static apr_status_t err_abort(void *data)
> > {
> > abort();
> > }
>
> A couple of remarks:
>
> * We should expect this to introduce a period of instability into our
> client when we first introduce it. Perhaps it should be limited to
> enable-maintainer-mode builds at least for a while.
>
> * If we ever enable it for non-maintainer-mode builds, perhaps spewing
> to stderr and exiting would be better than an abort() for those builds.
>
> * This will catch errors dropped by applications as well as errors we
> drop in our own libraries. (At least, if the applications perform an
> APR shutdown.) That might be considered a little unfriendly; there are
> many cases where an application doesn't need to care about a small
> amount of leaked memory, particularly when it only happens a constant
> number of times.
Hmm... good points, all. Must think on these things.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org