(patch) fix to properly terminate cscope without leaving temporary directory
Dominique Pelle
Using Vim-7.1.266 and cscope-15.6, I notice that
my /tmp/ directory gets cluttered with many temporary
directories /tmp/cscope.*/ (where * is a PID).
Every time I connect to a cscope database in Vim,
such a temporary directory is created and it is never
deleted. The temporary directory is created by the cscope
process spawned by Vim. However, it's a bug in Vim and
not in cscope. Vim does not properly clean up the
connection to cscope on Linux when exiting Vim or when
closing the connection to cscope with ":cs kill 0".
Here is how I can reproduce the bug:
1/ Start Vim
$ vim -u NONE
2/ Add a cscope DB (you need a cscope database for this)
:cs add ~/cscope.out
3/ Observe that a temporary file gets created (/tmp/cscope.14417 in my case)
(perhaps it goes in /var/tmp/ depending on your environment)
4/ Observe that it's cscope that created this temporary file:
$ ps -aux | grep 14417
pel 14417 0.0 0.0 2516 892 pts/3 S+ 11:26 0:00 cscope -dl -f
/home/pel/cscope.out
5/ Exit Vim with ":q"
6/ Observe that /tmp/cscope.PID temporary directory is still present.
I'd expect the temporary directory to be deleted when exiting Vim.
If I start cscope from the shell (with "cscope -d" or in line oriented
interface like what Vim does with "cscope -dl -f ~/cscope.out"), I see that
cscope creates a tmp directory too. However, when exiting cscope (with
CTRL-D in interactive mode or "q" in line oriented interface mode) the
temporary directory always gets deleted. So this directory does not get
deleted only when cscope is started from Vim.
Looking at Vim's code, I see that Vim kills cscope on Unix/Linux instead
of sending the q command to terminate it cleanly. The "q" command is
sent when it's _not_ UNIX. I can't see any good reason for that.
I attach a patch which fixes it.
Regards
/Dominique
PS: different topic: file src/testdir/test42.ok is still corrupted in CVS
Bram Moolenaar
Dominique Pelle wrote:
A few hints to improve this:
- Call alarm(0) to stop the alarm when cscope exits quickly.
- The default signal handler for SIGALRM is to ignore it. This needs to
be restored.
- sigaction() is not always available. Use signal(). See os_unix.c for
examples.
> PS: different topic: file src/testdir/test42.ok is still corrupted in CVS
Hmm, I thought I fixed it with:
cvs ... admin -kb src/testdir/test42.ok
What else can I do?
--
hundred-and-one symptoms of being an internet addict:
63. You start using smileys in your snail mail.
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ download, build and distribute -- http://www.A-A-P.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
Tony Mechelynck
>
> Dominique Pelle wrote:
[...]
>> PS: different topic: file src/testdir/test42.ok is still corrupted in CVS
>
> Hmm, I thought I fixed it with:
> cvs ... admin -kb src/testdir/test42.ok
> What else can I do?
>
re-upload the correct file to the CVS repository maybe? Patching the 7.1.000
file according to the published patches gives the correct result.
Best regards,
Tony.
--
Darth Vader sleeps with a Teddywookie.
Dominique Pelle
> Dominique Pelle wrote:
>
> > Using Vim-7.1.266 and cscope-15.6, I notice that
> > my /tmp/ directory gets cluttered with many temporary
> > directories /tmp/cscope.*/ (where * is a PID).
[...]
> > I attach a patch which fixes it.
>
> A few hints to improve this:
> - Call alarm(0) to stop the alarm when cscope exits quickly.
OK, done.
> - The default signal handler for SIGALRM is to ignore it. This needs to
> be restored.
OK, done.
> - sigaction() is not always available. Use signal(). See os_unix.c for
> examples.
I actually initially implemented it with signal() rather than sigaction().
It worked fine as long as cscope exits (unblocking waitpid())
but if cscope does not exit, SIGALRM did not unblock waitpid!?
If put a printf() in the signal handler, I could see the timeout happening,
yet it would not unblock waitpid(). With sigaction() on the other
hand it works fine, SIGALRM unblocks waitpid(). I've tested in normal
case when cscope exits normally (i.e. before timer expires) as well as
when cscope does not exits itself (i.e. when timer expires) which I
simulated by temporarily commenting the line that sends the "q"
command to cscope).
So everything works fine on my machine (Linux x86) with the
attached patch. But I cannot test on machines where HAVE_SIGACTION
is not defined. Hopefully you have a machine to test?
I have to say that setting up a timeout for waitpid() in a portable way
is a tad complicated and ugly: 3 different ways for various flavors of
Unix + another way for non Unix. I tried to make it as portable
as possible. If SIGALRM is not defined, it does not used timeout.
Perhaps the code using sigvec() is not needed? i.e. either use
sigaction() on Linux or signal() on other Unix flavors. That would
simplify a bit but I don't have the machine to test it.
> > PS: different topic: file src/testdir/test42.ok is still corrupted in CVS
>
> Hmm, I thought I fixed it with:
> cvs ... admin -kb src/testdir/test42.ok
> What else can I do?
Revision 1.2 looks corrupted somehow. Whether it was because of
CVS -kb option, I'm not sure. In any case, since it's a binary file, it
was better to set -kb option. How about checking it out and commiting
the pristine version again as revision 1.3?
Regards
Dominique
PS: I will be away for a week so I won't be able to touch the patch
further until then if something else needs to be improved.
Bram Moolenaar
Dominique Pelle wrote:
> > > Using Vim-7.1.266 and cscope-15.6, I notice that
> > > my /tmp/ directory gets cluttered with many temporary
> > > directories /tmp/cscope.*/ (where * is a PID).
> [...]
> > > I attach a patch which fixes it.
> >
> > A few hints to improve this:
> > - Call alarm(0) to stop the alarm when cscope exits quickly.
>
> OK, done.
>
> > - The default signal handler for SIGALRM is to ignore it. This needs to
> > be restored.
>
> OK, done.
>
> > - sigaction() is not always available. Use signal(). See os_unix.c for
> > examples.
>
> I actually initially implemented it with signal() rather than sigaction().
> It worked fine as long as cscope exits (unblocking waitpid())
> but if cscope does not exit, SIGALRM did not unblock waitpid!?
> If put a printf() in the signal handler, I could see the timeout happening,
> yet it would not unblock waitpid(). With sigaction() on the other
> hand it works fine, SIGALRM unblocks waitpid(). I've tested in normal
> case when cscope exits normally (i.e. before timer expires) as well as
> when cscope does not exits itself (i.e. when timer expires) which I
> simulated by temporarily commenting the line that sends the "q"
> command to cscope).
This must be because signal() works like sigaction() with the SA_RESTART
flag.
> So everything works fine on my machine (Linux x86) with the
> attached patch. But I cannot test on machines where HAVE_SIGACTION
> is not defined. Hopefully you have a machine to test?
Nope.
> I have to say that setting up a timeout for waitpid() in a portable way
> is a tad complicated and ugly: 3 different ways for various flavors of
> Unix + another way for non Unix. I tried to make it as portable
> as possible. If SIGALRM is not defined, it does not used timeout.
>
> Perhaps the code using sigvec() is not needed? i.e. either use
> sigaction() on Linux or signal() on other Unix flavors. That would
> simplify a bit but I don't have the machine to test it.
Why don't we use two methods: When sigaction() is available use that, as
in your first version of the patch. When it is not available then make
a loop that sleeps for a very short moment (e.g., 50ms, using
mch_delay()) and checks if cscope still runs. Break the loop after 2
seconds or when cscope has quit. That should be very portable.
See the proposed patch below, please try it out. I removed SA_NOMASK, my
system doesn't have it. I think SA_NODEFER is used instead. But can we
leave this one out completely?
> > > PS: different topic: file src/testdir/test42.ok is still corrupted in CVS
> >
> > Hmm, I thought I fixed it with:
> > cvs ... admin -kb src/testdir/test42.ok
> > What else can I do?
>
> Revision 1.2 looks corrupted somehow. Whether it was because of
> CVS -kb option, I'm not sure. In any case, since it's a binary file, it
> was better to set -kb option. How about checking it out and commiting
> the pristine version again as revision 1.3?
Ah, the copy of the file was already corrupted locally. Must have been
because "patch" failed on this file. Sorry for blaming CVS.
*** ../vim-7.1.266/src/if_cscope.c Fri Sep 14 19:56:18 2007
--- src/if_cscope.c Sat Mar 1 14:11:49 2008
***************
*** 2096,2101 ****
--- 2096,2113 ----
return CSCOPE_SUCCESS;
}
+ #if defined(UNIX) && defined(SIGALRM)
+ /*
+ * Used to catch and ignore SIGALRM below.
+ */
+ /* ARGSUSED */
+ static RETSIGTYPE
+ sig_handler SIGDEFARG(sigarg)
+ {
+ /* do nothing */
+ SIGRETURN;
+ }
+ #endif
/*
* PRIVATE: cs_release_csp
***************
*** 2108,2116 ****
int i;
int freefnpp;
{
- #if defined(UNIX)
- int pstat;
- #else
/*
* Trying to exit normally (not sure whether it is fit to UNIX cscope
*/
--- 2120,2125 ----
***************
*** 2119,2124 ****
--- 2128,2177 ----
(void)fputs("q\n", csinfo[i].to_fp);
(void)fflush(csinfo[i].to_fp);
}
+ #if defined(UNIX)
+ {
+ int pstat;
+ pid_t pid;
+
+ # if defined(HAVE_SIGACTION)
+ struct sigaction sa, old;
+
+ /* Use sigaction() to limit the waiting time to two seconds. */
+ sa.sa_handler = sig_handler;
+ sa.sa_flags = SA_NODEFER;
+ sigaction(SIGALRM, &sa, &old);
+ alarm(2); /* 2 sec timeout */
+
+ /* Block until cscope exits or until timer expires */
+ pid = waitpid(csinfo[i].pid, &pstat, 0);
+
+ /* cancel pending alarm if still there and restore signal */
+ alarm(0);
+ sigaction(SIGALRM, &old, NULL);
+ # else
+ int waited;
+
+ /* Can't use sigaction(), loop for two seconds. */
+ for (waited = 0; waited < 40; ++waited)
+ {
+ pid = waitpid(csinfo[i].pid, &pstat, WNOHANG);
+ if (pid > 0)
+ break;
+ mch_delay(50); /* sleep 50 ms */
+ }
+ # endif
+ /*
+ * If the cscope process is still running: kill it.
+ * Safety check: If the PID would be zero here, the entire X session
+ * would be killed. -1 and 1 are dangerous as well.
+ */
+ if (pid < 0 && csinfo[i].pid > 1)
+ {
+ kill(csinfo[i].pid, SIGTERM);
+ (void)waitpid(csinfo[i].pid, &pstat, 0);
+ }
+ }
+ #else /* !UNIX */
if (csinfo[i].hProc != NULL)
{
/* Give cscope a chance to exit normally */
***************
*** 2133,2150 ****
if (csinfo[i].to_fp != NULL)
(void)fclose(csinfo[i].to_fp);
- /*
- * Safety check: If the PID would be zero here, the entire X session would
- * be killed. -1 and 1 are dangerous as well.
- */
- #if defined(UNIX)
- if (csinfo[i].pid > 1)
- {
- kill(csinfo[i].pid, SIGTERM);
- (void)waitpid(csinfo[i].pid, &pstat, 0);
- }
- #endif
-
if (freefnpp)
{
vim_free(csinfo[i].fname);
--- 2186,2191 ----
--
hundred-and-one symptoms of being an internet addict:
69. Yahoo welcomes you with your own start page
Dominique Pelle
> Dominique Pelle wrote:
(...snip...)
> > I have to say that setting up a timeout for waitpid() in a portable way
> > is a tad complicated and ugly: 3 different ways for various flavors of
> > Unix + another way for non Unix. I tried to make it as portable
> > as possible. If SIGALRM is not defined, it does not used timeout.
> >
> > Perhaps the code using sigvec() is not needed? i.e. either use
> > sigaction() on Linux or signal() on other Unix flavors. That would
> > simplify a bit but I don't have the machine to test it.
>
> Why don't we use two methods: When sigaction() is available use that, as
> in your first version of the patch. When it is not available then make
> a loop that sleeps for a very short moment (e.g., 50ms, using
> mch_delay()) and checks if cscope still runs. Break the loop after 2
> seconds or when cscope has quit. That should be very portable.
>
> See the proposed patch below, please try it out. I removed SA_NOMASK, my
> system doesn't have it. I think SA_NODEFER is used instead. But can we
> leave this one out completely?
Yes, it looks simpler. Using asynchronous SIGALRM is
better of course. But when it's not possible, polling is a
pragmatic and portable fallback.
I can't try it because I'm away until next week. I see 2 refinements:
- If waitpid() returns -1 (error), the loop will currently retry 40 times.
I think this is a useless delay of 2 seconds. It's better
to break the loop immediately if waitpid returns -1.
In other words:
when pid == 0, sleep 50ms and retry
when pid > 0 exit loop (normal case when cscope finishes)
when pid < 0 error, break loop and revert to kill cscope.
- I think with the proposed patch, Vim will almost always
wait at least 50ms, because cscope did yet get a chance to
be scheduled before Vim calls the first unblocking waitpid(). So first
call to waitpid() will always return 0, and Vim will then always
call mch_delay(50) at least once (giving then a chance for cscope
to be scheduled) and second call to waitpid() is then likely to
return > 0. This may always add a small unfortunate delay
(but no much) when exiting Vim. If we add a sleep(0) before
first call to waitpid() to yield the CPU, it will give more chance
for cscope process to grab the CPU and thus respond to the
q command before Vim calls waitpid() for the first time. So the
first call to waitpid() has more chance to return with pid > 0,
hence possibly avoiding a sleep of 50ms. Of course they is
no guarantee, but it can only help I think.
Sorry, I can't provide a patch with this as I am away until
next week and I only have a webmail access.
> > > > PS: different topic: file src/testdir/test42.ok is still corrupted in CVS
> > >
> > > Hmm, I thought I fixed it with:
> > > cvs ... admin -kb src/testdir/test42.ok
> > > What else can I do?
> >
> > Revision 1.2 looks corrupted somehow. Whether it was because of
> > CVS -kb option, I'm not sure. In any case, since it's a binary file, it
> > was better to set -kb option. How about checking it out and commiting
> > the pristine version again as revision 1.3?
>
> Ah, the copy of the file was already corrupted locally. Must have been
> because "patch" failed on this file. Sorry for blaming CVS.
Thanks, I will check this when I'm back.
Cheers
-- Dominique
Bram Moolenaar
Dominique Pelle wrote:
I've included your suggestions. New patch:
*** ../vim-7.1.266/src/if_cscope.c Fri Sep 14 19:56:18 2007
--- src/if_cscope.c Sun Mar 2 13:31:43 2008
--- 2128,2179 ----
+ /* Can't use sigaction(), loop for two seconds. First yield the CPU
+ * to give cscope a chance to exit quickly. */
+ sleep(0);
+ for (waited = 0; waited < 40; ++waited)
+ {
+ pid = waitpid(csinfo[i].pid, &pstat, WNOHANG);
+ if (pid != 0)
+ break; /* break unless the process is still running */
--- 2188,2193 ----
--
Proof techniques #2: Proof by Oddity.
SAMPLE: To prove that horses have an infinite number of legs.
(1) Horses have an even number of legs.
(2) They have two legs in back and fore legs in front.
(3) This makes a total of six legs, which certainly is an odd number of
legs for a horse.
(4) But the only number that is both odd and even is infinity.
(5) Therefore, horses must have an infinite number of legs.
Dominique Pelle
I finally had time to look at this patch. When HAVE_SIGACTION is not
defined, I had to make one
small change and after that it worked fine.
Lines:
mch_delay(50); /* sleep 50 ms */
should be:
mch_delay(50, FALSE); /* sleep 50 ms */
-- Dominique