Loading ...
Sorry, an error occurred while loading the content.

Re: (patch) fix to properly terminate cscope without leaving temporary directory

Expand Messages
  • Dominique Pelle
    ... (...snip...) ... Yes, it looks simpler. Using asynchronous SIGALRM is better of course. But when it s not possible, polling is a pragmatic and portable
    Message 1 of 8 , Mar 2, 2008
    • 0 Attachment
      On Sat, Mar 1, 2008 at 2:04 PM, Bram Moolenaar <Bram@...> wrote:

      > 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

      --~--~---------~--~----~------------~-------~--~----~
      You received this message from the "vim_dev" maillist.
      For more information, visit http://www.vim.org/maillist.php
      -~----------~----~----~----~------~----~------~--~---
    • Bram Moolenaar
      ... I ve included your suggestions. New patch: ... *************** ... return CSCOPE_SUCCESS; } + #if defined(UNIX) && defined(SIGALRM) + /* + * Used to
      Message 2 of 8 , Mar 2, 2008
      • 0 Attachment
        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.

        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
        ***************
        *** 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,2179 ----
        (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. 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 */
        + 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);
        --- 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.

        /// Bram Moolenaar -- Bram@... -- 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 ///

        --~--~---------~--~----~------------~-------~--~----~
        You received this message from the "vim_dev" maillist.
        For more information, visit http://www.vim.org/maillist.php
        -~----------~----~----~----~------~----~------~--~---
      • 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:
        Message 3 of 8 , Mar 15, 2008
        • 0 Attachment
          On Sun, Mar 2, 2008 at 1:33 PM, Bram Moolenaar <Bram@...> wrote:
          >
          >
          > 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.
          >
          > 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
          >
          > ***************
          > *** 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,2179 ----
          >
          > (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. 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 */
          >
          > + 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);
          > --- 2188,2193 ----



          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

          --~--~---------~--~----~------------~-------~--~----~
          You received this message from the "vim_dev" maillist.
          For more information, visit http://www.vim.org/maillist.php
          -~----------~----~----~----~------~----~------~--~---
        Your message has been successfully submitted and would be delivered to recipients shortly.