Add retry logic to rebaseall

Add retry logic to rebaseall

[David Boyce]

[-- Attachment #1: Type: text/plain, Size: 801 bytes --]

Jason et al,

Here's a suggested new flag (with patch, attached) for
/usr/bin/rebaseall. It adds a -w(ait) flag which causes the check for
running Cygwin processes to be done in a loop, breaking out and doing
the rebaseall as soon as it finds a quiescent moment.

We run Cygwin on build servers which are almost always busy. When they
need a rebase (which is rare with modern Cygwin but has happened) it's
hard to find a moment when things are quiet. Of course we can take a
server out of the queue, wait for builds to finish, rebase, and put it
back but that's a slow, manual process. Running "dash -c
'/usr/bin/rebaseall -w 10'" would tell it to try every 10 seconds
until it sees an opening, then go for it and exit.

Arguably, the flag should be -r (retry) but I'll leave that to you if accepted.

[-- Attachment #2: rebaseall.diff --]
[-- Type: text/plain, Size: 1972 bytes --]

--- /bin/rebaseall	2013-12-03 16:28:03.000000000 -0800
+++ /bin/rebaseall.dsb	2014-01-09 09:51:12.338413000 -0800
@@ -29,7 +29,7 @@
 PATH=$(cd $tp2 && pwd):/usr/bin:/bin
 
 ProgramName=${0##*/}
-ProgramOptions='48b:o:ps:tT:v'
+ProgramOptions='48b:o:ps:tT:vw:'
 DefaultBaseAddress=0x70000000
 DefaultOffset=0
 DefaultTouch=
@@ -41,7 +41,7 @@
 # Define functions
 usage()
 {
-    echo "usage: ${ProgramName} [-b BaseAddress] [-o Offset] [-s DllSuffix] [-T FileList | -] [-4|-8] [-p] [-t] [-v]"
+    echo "usage: ${ProgramName} [-b BaseAddress] [-o Offset] [-s DllSuffix] [-T FileList | -] [-4|-8] [-p] [-t] [-v] [-w seconds]"
     exit 1
 }
 
@@ -60,6 +60,7 @@
 Touch="${DefaultTouch}"
 NoDyn="${DefaultNoDyn}"
 Verbose="${DefaultVerbose}"
+WaitSeconds=0
 FileList="${DefaultFileList}"
 Suffixes="${DefaultSuffixes}"
 db_file_i386="/etc/rebase.db.i386"
@@ -123,11 +124,15 @@
 	FileList="${OPTARG}";;
     v)
 	Verbose="-v";;
+    w)
+	WaitSeconds="${OPTARG}";;
     \?)
 	usage;;
     esac
 done
 
+while :
+do
 # Verify only ash or dash processes are running
 if [ "${check_for_dash_only}" != "no" ]
 then
@@ -160,15 +165,25 @@
       ProcessResult=$?
       ;;
   esac
-  if [ $ProcessResult -eq 0 -a -z "${RebaseDebug}" ]
+      # If none found, we're good to go.
+      if [ $ProcessResult -ne 0 -o -n "${RebaseDebug}" ]
+      then
+	  break
+      fi
+      # Otherwise either quit or wait and retry.
+      if [ $WaitSeconds -eq 0 ]
   then
       echo "${ProgramName}: only ash or dash processes are allowed during rebasing"
       echo "    Exit all Cygwin processes and stop all Cygwin services."
       echo "    Execute ash (or dash) from Start/Run... or a cmd or command window."
       echo "    Execute '/bin/rebaseall' from ash (or dash)."
       exit 2
+      else
+	  echo "${ProgramName}: Cygwin processes found, retry in $WaitSeconds seconds ..."
+	  sleep $WaitSeconds
   fi
 fi
+done
 
 # Check if rebase database already exists.
 database_exists="no"

[-- Attachment #3: Type: text/plain, Size: 218 bytes --]

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: Add retry logic to rebaseall

[Christopher Faylor]

On Wed, Jan 15, 2014 at 11:23:08PM -0500, David Boyce wrote:
>Jason et al,
>
>Here's a suggested new flag (with patch, attached) for
>/usr/bin/rebaseall. It adds a -w(ait) flag which causes the check for
>running Cygwin processes to be done in a loop, breaking out and doing
>the rebaseall as soon as it finds a quiescent moment.

+1

Seems like a good idea to me.

cgf

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: Add retry logic to rebaseall

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 655 bytes --]

On Jan 16 01:19, Christopher Faylor wrote:
> On Wed, Jan 15, 2014 at 11:23:08PM -0500, David Boyce wrote:
> >Jason et al,
> >
> >Here's a suggested new flag (with patch, attached) for
> >/usr/bin/rebaseall. It adds a -w(ait) flag which causes the check for
> >running Cygwin processes to be done in a loop, breaking out and doing
> >the rebaseall as soon as it finds a quiescent moment.
> 
> +1
> 
> Seems like a good idea to me.

ACK.  Jason, shall I apply the patch?


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: Add retry logic to rebaseall

[Jason Tishler]

On Thu, Jan 16, 2014 at 09:54:31AM +0100, Corinna Vinschen wrote:
> On Jan 16 01:19, Christopher Faylor wrote:
> > On Wed, Jan 15, 2014 at 11:23:08PM -0500, David Boyce wrote:
> > >Jason et al,
> > >
> > >Here's a suggested new flag (with patch, attached) for
> > >/usr/bin/rebaseall. It adds a -w(ait) flag which causes the check for
> > >running Cygwin processes to be done in a loop, breaking out and doing
> > >the rebaseall as soon as it finds a quiescent moment.
> > 
> > +1
> > 
> > Seems like a good idea to me.
> 
> ACK.  Jason, shall I apply the patch?

AFAICT, there is a race condition issue with the proposed functionality.
David's build servers could be quiescent when the check for running
processes is performed, but they could restart before the rebase is
finished.  I realize the window is very small, but it is nevertheless
nonzero, so the rebase could still fail.

The patch could be enhanced to check the status of the rebase and retry
if the rebase failed and the wait option was specified.  This would fix
the race condition.

There are also formatting issues with the patch.  For example, the
addition of the while loop requires lines to be shifted to the right.
I know rebaseall unfortunately has a mixture of tabs and spaces, but
after applying the patch some lines are not indented correctly.

IMO, the proposed functionality is very specialized and doesn't seem to
be generally applicable.  This functionality could also be implemented
(by the few who need it) as a very simple wrapper script that calls
rebaseall until is succeeds.  This approach would also workaround the
race condition.

I'm not enamored with this change, but I will go along with the
consensus.  If we decide to move forward, then I would like the race
condition and formatting issues resolved.

Thanks,
Jason

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: Add retry logic to rebaseall

[David Boyce]

Jason,

On Fri, Jan 17, 2014 at 11:13 AM, Jason Tishler <jason@tishler.net> wrote:
> AFAICT, there is a race condition issue with the proposed functionality.
> David's build servers could be quiescent when the check for running
> processes is performed, but they could restart before the rebase is
> finished.  I realize the window is very small, but it is nevertheless
> nonzero, so the rebase could still fail.

I don't think race condition is the right phrase here. This is the
exact same situation faced by the existing rebaseall functionality; it
knows there are no Cygwin processes running at start time but any
process could start between then and end time. I agree there's a
window where things could go wrong, but this feature doesn't worsen
it. Closing that window is an orthogonal effort, IMHO; the new flag
isn't called --make-sure-the-rebase-works-dammit.

> There are also formatting issues with the patch.  For example, the
> addition of the while loop requires lines to be shifted to the right.
> I know rebaseall unfortunately has a mixture of tabs and spaces, but
> after applying the patch some lines are not indented correctly.

I had a paragraph about that in the original email which got rejected
due to "spam score too high" so I cut the text down for the second try
and ended up losing that part. Yes, the original has a mix of tabs and
spaces and my editor might be configured differently from yours, so I
made the patch using "diff -u -w". That may have been a mistake; I'm
happy to clean it up if asked.

> IMO, the proposed functionality is very specialized and doesn't seem to
> be generally applicable.  This functionality could also be implemented
> (by the few who need it) as a very simple wrapper script that calls
> rebaseall until is succeeds.  This approach would also workaround the
> race condition.

How so? The behavior and risk factors would be identical as far as I can see.

David

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: Add retry logic to rebaseall

[Jason Tishler]

David,

On Fri, Jan 17, 2014 at 11:30:54AM -0500, David Boyce wrote:
> On Fri, Jan 17, 2014 at 11:13 AM, Jason Tishler <jason@tishler.net> wrote:
> > AFAICT, there is a race condition issue with the proposed
> > functionality.  David's build servers could be quiescent when the
> > check for running processes is performed, but they could restart
> > before the rebase is finished.  I realize the window is very small,
> > but it is nevertheless nonzero, so the rebase could still fail.
> 
> I don't think race condition is the right phrase here. This is the
> exact same situation faced by the existing rebaseall functionality; it
> knows there are no Cygwin processes running at start time but any
> process could start between then and end time.

Yes, but rebaseall it typically called manually, so the user wouldn't do
this or would have to deal with the consequences if they did.

> I agree there's a window where things could go wrong, but this feature
> doesn't worsen it. Closing that window is an orthogonal effort, IMHO;
> the new flag isn't called --make-sure-the-rebase-works-dammit.

IMO, if a feature adds automation (i.e., no human intervention required),
then it should try to work under all conditions, if feasible without too
much effort.  Your wait feature could also detect rebase failures and
retry until it succeeds (maybe giving up after N attempts).

> > There are also formatting issues with the patch.  For example, the
> > addition of the while loop requires lines to be shifted to the right.
> > I know rebaseall unfortunately has a mixture of tabs and spaces, but
> > after applying the patch some lines are not indented correctly.
> 
> I had a paragraph about that in the original email which got rejected
> due to "spam score too high" so I cut the text down for the second try
> and ended up losing that part. Yes, the original has a mix of tabs and
> spaces and my editor might be configured differently from yours, so I
> made the patch using "diff -u -w". That may have been a mistake; I'm
> happy to clean it up if asked.

I agree that the formatting issue is minor, but a patch should be
generated to minimize a maintainer's efforts.

> > IMO, the proposed functionality is very specialized and doesn't seem
> > to be generally applicable.  This functionality could also be
> > implemented (by the few who need it) as a very simple wrapper script
> > that calls rebaseall until is succeeds.  This approach would also
> > workaround the race condition.
> 
> How so? The behavior and risk factors would be identical as far as I
> can see.

If you wrap rebaseall in its entirety and retry on failure until
successful, then that will eliminate the "race condition" issue.

If consensus thinks this change is generally useful, then rebaseall
should be changed accordingly.  Otherwise, you can write a few line
wrapper script to meet your special needs.

Jason

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: Add retry logic to rebaseall

[Christopher Faylor]

On Fri, Jan 17, 2014 at 12:45:47PM -0500, Jason Tishler wrote:
>On Fri, Jan 17, 2014 at 11:30:54AM -0500, David Boyce wrote:
>If you wrap rebaseall in its entirety and retry on failure until
>successful, then that will eliminate the "race condition" issue.
>
>If consensus thinks this change is generally useful, then rebaseall
>should be changed accordingly.  Otherwise, you can write a few line
>wrapper script to meet your special needs.

My 2c: I think it's likely that other people will find this kind
of functionality useful so, rather than introduce YA script to do
this, I think modifying rebaseall makes sense.  Then we can just
point people to using rebaseall -w.

It might even make sense to use this when setup.exe runs rebaseall.

Again, just my 2c opinion.

cgf

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple