You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2016/01/30 08:47:08 UTC
Re: svn commit: r1727621 - in /subversion/trunk/subversion:
svn/svn.c svnadmin/svnadmin.c svnbench/svnbench.c svnfsfs/svnfsfs.c
svnlook/svnlook.c svnrdump/svnrdump.c svnsync/svnsync.c
philip@apache.org wrote on Fri, Jan 29, 2016 at 18:55:22 -0000:
> +#ifndef WIN32
> + /* If cancelled by SIGINT then attempt to exit via SIGINT. This
> + allows the shell to use WIFSIGNALED and WTERMSIG to detect the
> + SIGINT. See http://www.cons.org/cracauer/sigint.html */
> + if (cancelled && apr_signal(SIGINT, SIG_DFL) == APR_SUCCESS)
> + /* No APR support for getpid() so cannot use apr_proc_kill(). */
> + kill(getpid(), SIGINT);
> +#endif
Should this be done for other signals we catch? (Those signals are
SIGTERM, SIGHUP, and SIGBREAK.) Shells handle those values of
WTERMSIG() too:
% perl -e 'kill "TERM", $$'
zsh: terminated perl -e 'kill "TERM", $$'
Re: svn commit: r1727621 - in /subversion/trunk/subversion: svn/svn.c svnadmin/svnadmin.c svnbench/svnbench.c svnfsfs/svnfsfs.c svnlook/svnlook.c svnrdump/svnrdump.c svnsync/svnsync.c
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> That change is broken for two reasons:
>
> - the same handler is used for all those signals but we always exit via
> SIGINT
>
> - except apr_signal does not return APR_SUCCESS so we never exit via
> SIGINT.
Fixed by r1727916.
To see the effect of this run the following at a sh prompt:
while :;do echo sleep;sleep 3;echo log;svn log http://svn.apache.org/repos/asf;done
Using control-C to send SIGINT will exit the loop only if sent while
sleep is running. A SIGINT during log will cancel the svn command but
the loop will continue and a second SIGINT during sleep will be needed
to exit the loop.
--
Philip Martin
WANdisco
Re: svn commit: r1727621 - in /subversion/trunk/subversion: svn/svn.c svnadmin/svnadmin.c svnbench/svnbench.c svnfsfs/svnfsfs.c svnlook/svnlook.c svnrdump/svnrdump.c svnsync/svnsync.c
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> That change is broken for two reasons:
>
> - the same handler is used for all those signals but we always exit via
> SIGINT
>
> - except apr_signal does not return APR_SUCCESS so we never exit via
> SIGINT.
Fixed by r1727916.
To see the effect of this run the following at a sh prompt:
while :;do echo sleep;sleep 3;echo log;svn log http://svn.apache.org/repos/asf;done
Using control-C to send SIGINT will exit the loop only if sent while
sleep is running. A SIGINT during log will cancel the svn command but
the loop will continue and a second SIGINT during sleep will be needed
to exit the loop.
--
Philip Martin
WANdisco
Re: Moving cancellation support to libsvn_subr
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Tue, Feb 09, 2016 at 18:19:02 +0000:
> This code keeps growing and I'm thinking of moving both the signal
> handler and cancellation handler to libsvn_subr. We need some storage
> for the volatiles and using pool memory implies a limited lifetime that
> interacts badly with the signal handlers so I'm thinking of using static
> storage in the library. Something like:
>
> /* Setup signal handlers and return a cancellation handler. */
> svn_cancel_func_t svn_cmdline__setup_cancellation_handler();
>
> /* Return a signal that triggered cancellation or zero if no such
> signal. */
> int svn_cmdline__get_cancelled_signum();
For future reference, Philip implemented that in r1729463.
Moving cancellation support to libsvn_subr
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> This fixes the race I mentioned, but I think we have another problem
> here: signum_cancelled is accessed by the signal handler so it must be
> of type 'static volatile sig_atomic_t' (C89 §4.7.1.1)... but I don't
> think anything guarantees that signal numbers are representable as
> a sig_atomic_t. On Linux/amd64 and FreeBSD they are, but that need not
> be the case everywhere.
>
> If we can't assume sig_atomic_t is at least as large as an int, I think
> we have to use the following roundabout method:
I did something similar in r1729422.
This code keeps growing and I'm thinking of moving both the signal
handler and cancellation handler to libsvn_subr. We need some storage
for the volatiles and using pool memory implies a limited lifetime that
interacts badly with the signal handlers so I'm thinking of using static
storage in the library. Something like:
/* Setup signal handlers and return a cancellation handler. */
svn_cancel_func_t svn_cmdline__setup_cancellation_handler();
/* Return a signal that triggered cancellation or zero if no such
signal. */
int svn_cmdline__get_cancelled_signum();
and the application would do:
int main()
{
apr_pool_t *pool;
...
SVN_ERR(svn_client_create_context2(&ctx, ..., pool));
...
ctx->cancel_func = svn_cmdline__setup_cancellation_handler();
...
svn_pool_destroy(pool);
...
#ifndef WIN32
{
int signum = svn_cmdline__get_cancelled_signum();
if (signum)
kill(getpid(), signum);
}
#endif
return exit_code;
}
--
Philip Martin
WANdisco
Re: svn commit: r1727621 - in /subversion/trunk/subversion:
svn/svn.c svnadmin/svnadmin.c svnbench/svnbench.c svnfsfs/svnfsfs.c
svnlook/svnlook.c svnrdump/svnrdump.c svnsync/svnsync.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Mon, Feb 08, 2016 at 10:33:05 +0000:
> When there are two different signals I don't think there is a 'correct'
> signal with which to exit, all that matters is that we exit with one of
> the signals.
Agreed.
> So how about:
>
> int saved_signum = signum_cancelled;
> apr_signal(saved_signum, SIG_DFL);
> kill(getpid(), saved_signum);
This fixes the race I mentioned, but I think we have another problem
here: signum_cancelled is accessed by the signal handler so it must be
of type 'static volatile sig_atomic_t' (C89 §4.7.1.1)... but I don't
think anything guarantees that signal numbers are representable as
a sig_atomic_t. On Linux/amd64 and FreeBSD they are, but that need not
be the case everywhere.
If we can't assume sig_atomic_t is at least as large as an int, I think
we have to use the following roundabout method:
static volatile sig_atomic_t signum_cancelled[4] = {0, 0, 0, 0};
static void signal_handler(int signum)
{
⋮
if (signum == SIGINT)
signum_cancelled[0] = TRUE;
#ifdef SIGHUP
if (signum == SIGHUP)
signum_cancelled[1] = TRUE;
#endif
#ifdef SIGTERM
if (signum == SIGTERM)
signum_cancelled[2] = TRUE;
#endif
#ifdef SIGBREAK
if (signum == SIGBREAK)
signum_cancelled[3] = TRUE;
#endif
}
and then check which element(s) of the array are set to know which
signal to send to self.
Cheers,
Daniel
Re: svn commit: r1727621 - in /subversion/trunk/subversion: svn/svn.c svnadmin/svnadmin.c svnbench/svnbench.c svnfsfs/svnfsfs.c svnlook/svnlook.c svnrdump/svnrdump.c svnsync/svnsync.c
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> I think we have two options to fix this: either make signum_cancelled
> a write-once variable (never write to it if it's nonzero), or have the
> handler install SIG_IGN handlers for all signals we catch, not just for
> one of them. Or perhaps both? The former approach seems more robust
> but the latter seems like a good idea in its own right.
There is a race right at the end but there is also a race with a much
bigger window earlier. When a signal, say SIGINT, causes the program to
begin the process of cancelling then a second signal, say SIGTERM, can
arrive before we get to the exit code.
When there are two different signals I don't think there is a 'correct'
signal with which to exit, all that matters is that we exit with one of
the signals. So how about:
int saved_signum = signum_cancelled;
apr_signal(saved_signum, SIG_DFL);
kill(getpid(), saved_signum);
--
Philip Martin
WANdisco
Re: svn commit: r1727621 - in /subversion/trunk/subversion:
svn/svn.c svnadmin/svnadmin.c svnbench/svnbench.c svnfsfs/svnfsfs.c
svnlook/svnlook.c svnrdump/svnrdump.c svnsync/svnsync.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Mon, Feb 01, 2016 at 11:20:04 +0000:
> Philip Martin <ph...@wandisco.com> writes:
>
> > That change is broken for two reasons:
> >
> > - the same handler is used for all those signals but we always exit via
> > SIGINT
> >
> > - except apr_signal does not return APR_SUCCESS so we never exit via
> > SIGINT.
>
> Fixed by r1727916.
> + if (cancelled)
> + {
> + apr_signal(signum_cancelled, SIG_DFL);
> + /* No APR support for getpid() so cannot use apr_proc_kill(). */
> + kill(getpid(), signum_cancelled);
> + }
There seems to be a race condition here: if this block is entered with
(signum_cancelled == SIGINT), and between the apr_signal() call and
evaluating the arguments to the kill() call, a SIGTERM is received and
changes the value of signum_cancelled, the kill() call will send
a SIGTERM signal to current process while the SIGTERM handler will still
be SIG_IGN (the value installed by our handler).
I think we have two options to fix this: either make signum_cancelled
a write-once variable (never write to it if it's nonzero), or have the
handler install SIG_IGN handlers for all signals we catch, not just for
one of them. Or perhaps both? The former approach seems more robust
but the latter seems like a good idea in its own right.
> To see the effect of this run the following at a sh prompt:
>
> while :;do echo sleep;sleep 3;echo log;svn log http://svn.apache.org/repos/asf;done
>
> Using control-C to send SIGINT will exit the loop only if sent while
> sleep is running. A SIGINT during log will cancel the svn command but
> the loop will continue and a second SIGINT during sleep will be needed
> to exit the loop.
Cheers,
Daniel
Re: svn commit: r1727621 - in /subversion/trunk/subversion:
svn/svn.c svnadmin/svnadmin.c svnbench/svnbench.c svnfsfs/svnfsfs.c
svnlook/svnlook.c svnrdump/svnrdump.c svnsync/svnsync.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Mon, Feb 01, 2016 at 11:20:04 +0000:
> Philip Martin <ph...@wandisco.com> writes:
>
> > That change is broken for two reasons:
> >
> > - the same handler is used for all those signals but we always exit via
> > SIGINT
> >
> > - except apr_signal does not return APR_SUCCESS so we never exit via
> > SIGINT.
>
> Fixed by r1727916.
> + if (cancelled)
> + {
> + apr_signal(signum_cancelled, SIG_DFL);
> + /* No APR support for getpid() so cannot use apr_proc_kill(). */
> + kill(getpid(), signum_cancelled);
> + }
There seems to be a race condition here: if this block is entered with
(signum_cancelled == SIGINT), and between the apr_signal() call and
evaluating the arguments to the kill() call, a SIGTERM is received and
changes the value of signum_cancelled, the kill() call will send
a SIGTERM signal to current process while the SIGTERM handler will still
be SIG_IGN (the value installed by our handler).
I think we have two options to fix this: either make signum_cancelled
a write-once variable (never write to it if it's nonzero), or have the
handler install SIG_IGN handlers for all signals we catch, not just for
one of them. Or perhaps both? The former approach seems more robust
but the latter seems like a good idea in its own right.
> To see the effect of this run the following at a sh prompt:
>
> while :;do echo sleep;sleep 3;echo log;svn log http://svn.apache.org/repos/asf;done
>
> Using control-C to send SIGINT will exit the loop only if sent while
> sleep is running. A SIGINT during log will cancel the svn command but
> the loop will continue and a second SIGINT during sleep will be needed
> to exit the loop.
Cheers,
Daniel
Re: svn commit: r1727621 - in /subversion/trunk/subversion: svn/svn.c svnadmin/svnadmin.c svnbench/svnbench.c svnfsfs/svnfsfs.c svnlook/svnlook.c svnrdump/svnrdump.c svnsync/svnsync.c
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> philip@apache.org wrote on Fri, Jan 29, 2016 at 18:55:22 -0000:
>> +#ifndef WIN32
>> + /* If cancelled by SIGINT then attempt to exit via SIGINT. This
>> + allows the shell to use WIFSIGNALED and WTERMSIG to detect the
>> + SIGINT. See http://www.cons.org/cracauer/sigint.html */
>> + if (cancelled && apr_signal(SIGINT, SIG_DFL) == APR_SUCCESS)
>> + /* No APR support for getpid() so cannot use apr_proc_kill(). */
>> + kill(getpid(), SIGINT);
>> +#endif
>
> Should this be done for other signals we catch? (Those signals are
> SIGTERM, SIGHUP, and SIGBREAK.) Shells handle those values of
> WTERMSIG() too:
>
> % perl -e 'kill "TERM", $$'
> zsh: terminated perl -e 'kill "TERM", $$'
That change is broken for two reasons:
- the same handler is used for all those signals but we always exit via
SIGINT
- except apr_signal does not return APR_SUCCESS so we never exit via
SIGINT.
--
Philip Martin
WANdisco
Re: svn commit: r1727621 - in /subversion/trunk/subversion: svn/svn.c svnadmin/svnadmin.c svnbench/svnbench.c svnfsfs/svnfsfs.c svnlook/svnlook.c svnrdump/svnrdump.c svnsync/svnsync.c
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> philip@apache.org wrote on Fri, Jan 29, 2016 at 18:55:22 -0000:
>> +#ifndef WIN32
>> + /* If cancelled by SIGINT then attempt to exit via SIGINT. This
>> + allows the shell to use WIFSIGNALED and WTERMSIG to detect the
>> + SIGINT. See http://www.cons.org/cracauer/sigint.html */
>> + if (cancelled && apr_signal(SIGINT, SIG_DFL) == APR_SUCCESS)
>> + /* No APR support for getpid() so cannot use apr_proc_kill(). */
>> + kill(getpid(), SIGINT);
>> +#endif
>
> Should this be done for other signals we catch? (Those signals are
> SIGTERM, SIGHUP, and SIGBREAK.) Shells handle those values of
> WTERMSIG() too:
>
> % perl -e 'kill "TERM", $$'
> zsh: terminated perl -e 'kill "TERM", $$'
That change is broken for two reasons:
- the same handler is used for all those signals but we always exit via
SIGINT
- except apr_signal does not return APR_SUCCESS so we never exit via
SIGINT.
--
Philip Martin
WANdisco