5.0.9 eventually...?

5.0.9 eventually...?

[Peter Stephenson]

I've just returned from two weeks' unexpected absence, where I've been
trying to follow emails but have otherwise had limited connectivity.

Anyone (including Bart, obviously) with a feel for what's the state of
affairs with regards to immediate problems?

Is the process problem in 35955 etc. still there?

pws

Re: 5.0.9 eventually...?

[Bart Schaefer]

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

On Aug 8, 2015 12:11 PM, "Peter Stephenson" <p.w.stephenson@ntlworld.com>
wrote:
>
> Anyone (including Bart, obviously) with a feel for what's the state of
> affairs with regards to immediate problems?

Nothing has been done about the infinite loop with "wait" printing the job
status.  After pointing out that ksh and bash differ in wait
implementation, I got distracted by the signal handing deadlock thread.

I have what I think is a comprehensive patch for that signal issue but am
AFK today (writing this message on my phone).  Will post later.

I posted a sort-of patch for the "typeset -T" crash, but I agree that it is
not the ideal solution.

That's as much as I'm aware of from memory.

Re: 5.0.9 eventually...?

[Mikael Magnusson]

On Sat, Aug 8, 2015 at 10:40 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Aug 8, 2015 12:11 PM, "Peter Stephenson" <p.w.stephenson@ntlworld.com>
> wrote:
>>
>> Anyone (including Bart, obviously) with a feel for what's the state of
>> affairs with regards to immediate problems?
>
> Nothing has been done about the infinite loop with "wait" printing the job
> status.  After pointing out that ksh and bash differ in wait
> implementation, I got distracted by the signal handing deadlock thread.
>
> I have what I think is a comprehensive patch for that signal issue but am
> AFK today (writing this message on my phone).  Will post later.
>
> I posted a sort-of patch for the "typeset -T" crash, but I agree that it is
> not the ideal solution.

As far as I could tell, the code in question has been there since 2005
so it's not a recent regression, if that's a factor.

> That's as much as I'm aware of from memory.

Same, and glancing through the threads, I can't see anything else
important either.

All the (known) $(( ))-related problems were fixed, right?

-- 
Mikael Magnusson

Re: 5.0.9 eventually...?

[Oliver Kiddle]

Peter wrote:
> Anyone (including Bart, obviously) with a feel for what's the state of
> affairs with regards to immediate problems?

Has anything been done about the bug reported in 35852? I was guessing
that was related to typeset changes.

I'll apply 35834 (conditional stripping a final newline on bracketed
paste) because I think it is better than nothing. The PS2 idea would be
better but is not trivial. I'll also make an exception for vared and a
paste consisting of only a newline. There was also a couple of other
things I should finish but nothing that need hold up 5.0.9.

Oliver

Re: 5.0.9 eventually...?

[Bart Schaefer]

On Aug 8, 10:50pm, Mikael Magnusson wrote:
}
} All the (known) $(( ))-related problems were fixed, right?

I believe so, and I think I squashed the "for (( ... ))" one.

Re: 5.0.9 eventually...?

[Bart Schaefer]

On Aug 8, 11:22pm, Oliver Kiddle wrote:
}
} Has anything been done about the bug reported in 35852? I was guessing
} that was related to typeset changes.

No, nothing has been done about that.  It is indeed new in 5.0.8, so the
typeset changes are a likely place to look.

Re: 5.0.9 eventually...?

[Bart Schaefer]

} } Has anything been done about the bug reported in 35852?
} 
} No, nothing has been done about that.  It is indeed new in 5.0.8

That was supposed to say "5.0.8-dev-1".

Re: 5.0.9 eventually...?

[Jun T.]

2015/08/09 09:24, Bart Schaefer <schaefer@brasslantern.com> wrote:

> } } Has anything been done about the bug reported in 35852?
> } 
> } No, nothing has been done about that.  It is indeed new in 5.0.8
> 
> That was supposed to say "5.0.8-dev-1".

35852 ? Do you mean this?

Subject: Completion bug after assignment,unset
>    zsh -f
>    autoload compinit && compinit
>    RPS2=$PS2; unset PS2 ; setopt <TAB>
> 
> completes files rather than options.

In my case it completes commands (not files), while
% a=b; setopt <TAB>
completes files.

Anyway, this problem seems to be much older.
git-bisect indicates that it has started with the following commit:

commit 568e0db7a964feefa45061967d0c7079a0e59c1e
Author: Peter Stephenson <p.w.stephenson@ntlworld.com>
Date:   Fri Aug 2 18:58:35 2013 +0100

    31611: attempt to fix crash completing redirection in do loop


I can't understand how get_comp_string() works, of course...

Completion after assignment as command (Re: 5.0.9 eventually...?)

[Peter Stephenson]

On Sun, 9 Aug 2015 23:45:00 +0900
"Jun T." <takimoto-j@kba.biglobe.ne.jp> wrote:
> 35852 ? Do you mean this?
> 
> Subject: Completion bug after assignment,unset
> >    zsh -f
> >    autoload compinit && compinit
> >    RPS2=$PS2; unset PS2 ; setopt <TAB>
> > 
> > completes files rather than options.
> 
> In my case it completes commands (not files), while
> % a=b; setopt <TAB>
> completes files.
>...
> I can't understand how get_comp_string() works, of course...

No one does, which is why it was changed in the slightly fudged way it
was to minimise the overall effect instead of to fix the probalem
outright.

The following improves things without any obvious side effect, maybe ---
I only just noticed the !incond thing at the last minute, which would
otherwise have screwed up completion in conditions which might have
taken a while to notice.  Note I've remvoed some unnecessary parentheses
just as a very small concession to clartiy.

I don't understand the SEPER / ins handling except to the limited extent
I've noted in previous tortuous and frustrated comments.  Any problems
with this?  Anyone any idea how to add a test for it?

pws

diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index 3bf8d45..b87b99b 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -1175,9 +1175,19 @@ get_comp_string(void)
     do {
         qsub = noword = 0;
 
-	lincmd = ((incmdpos && !ins && !incond) ||
-		  (oins == 2 && wordpos == 2) ||
-		  (ins == 3 && wordpos == 1));
+	/*
+	 * pws: added cmdtok == NULLTOK test as fallback for detecting
+	 * we haven't had a command yet.  This is a cop out: it's needed
+	 * after SEPER because of bizarre and incomprehensible dance
+	 * that we otherwise do involving the "ins" flag when you might
+	 * have thought we'd just reset everything because we're now
+	 * considering a new command.  Consequently, although this looks
+	 * relatively harmless by itself, it's probably incomplete.
+	 */
+	lincmd = (incmdpos && !ins && !incond) ||
+	    (oins == 2 && wordpos == 2) ||
+	    (ins == 3 && wordpos == 1) ||
+	    (cmdtok == NULLTOK && !incond);
 	linredir = (inredir && !ins);
 	oins = ins;
 	/* Get the next token. */

Re: 5.0.9 eventually...?

[Ismail Donmez]

Peter Stephenson <p.w.stephenson <at> ntlworld.com> writes:

> 
> I've just returned from two weeks' unexpected absence, where I've been
> trying to follow emails but have otherwise had limited connectivity.
> 
> Anyone (including Bart, obviously) with a feel for what's the state of
> affairs with regards to immediate problems?

latest git started to fail here on Linux x64:

./A05execution.ztst: starting.
This test takes 5 seconds to fail...
Test ./A05execution.ztst failed: bad status 2, expected 1 from:
  { unsetopt MONITOR } 2>/dev/null
  (exit 1) &
  one=$!
  (exit 2) &
  two=$!
  (exit 3) &
  three=$!
  wait $three
  print $?
  wait $two
  print $?
  wait $one
Was testing: The status of recently exited background jobs is recorded

Re: 5.0.9 eventually...?

[Ismail Donmez]

Ismail Donmez <ismail <at> donmez.ws> writes:

> latest git started to fail here on Linux x64:
> 
> ./A05execution.ztst: starting.
> This test takes 5 seconds to fail...
> Test ./A05execution.ztst failed: bad status 2, expected 1 from:

FWIW reverting workers/36022 fixes the problem here.

Wait test failure

[Peter Stephenson]

On Mon, 10 Aug 2015 07:12:41 +0000
Ismail Donmez <ismail@donmez.ws> wrote:
> latest git started to fail here on Linux x64:
> 
> ./A05execution.ztst: starting.
> This test takes 5 seconds to fail...
> Test ./A05execution.ztst failed: bad status 2, expected 1 from:
>   { unsetopt MONITOR } 2>/dev/null
>   (exit 1) &
>   one=$!
>   (exit 2) &
>   two=$!
>   (exit 3) &
>   three=$!
>   wait $three
>   print $?
>   wait $two
>   print $?
>   wait $one
> Was testing: The status of recently exited background jobs is recorded

Not a fix, but this makes the failure clearer: it really is only that
last "wait" returning the wrong status, 2 instead of 1.

pws

diff --git a/Test/A05execution.ztst b/Test/A05execution.ztst
index cc2d34d..d5501bb 100644
--- a/Test/A05execution.ztst
+++ b/Test/A05execution.ztst
@@ -266,9 +266,11 @@ F:anonymous function, and a descriptor leak when backgrounding a pipeline
   wait $two
   print $?
   wait $one
-1:The status of recently exited background jobs is recorded
+  print $?
+0:The status of recently exited background jobs is recorded
 >3
 >2
+>1
 
 # Regression test for workers/34060 (patch in 34065)
   setopt ERR_EXIT NULL_GLOB

Re: Wait test failure

[Peter Stephenson]

Adding this code at the end of addbgstatus() in jobs.c

    fprintf(stderr, "Added bgstatus %d for %d\n", status, pid);
    fflush(stderr);

shows it's adding the wrong status (2) at the start:

Added bgstatus 2 for 24507
Added bgstatus 2 for 24506
Added bgstatus 3 for 24508

i.e. the problem isn't in the wait, it's already got the wrong status
when the process exits.  However, running this from the command line
doesn't show the problem, even with MONITOR turned off.

I'm wondering if in

  (exit 1) &
  one=$!
  (exit 2) &
  two=$!

when we handle the first background process, sometimes the second has
already exited so we use the status from that instead?  That would be
quite serious --- and quite possibly not a new effect, just exacerbated
by additional signal blocking.

pws

Re: Wait test failure

[Peter Stephenson]

On Mon, 10 Aug 2015 16:18:17 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> 
> I'm wondering if in
> 
>   (exit 1) &
>   one=$!
>   (exit 2) &
>   two=$!
> 
> when we handle the first background process, sometimes the second has
> already exited so we use the status from that instead?  That would be
> quite serious --- and quite possibly not a new effect, just exacerbated
> by additional signal blocking.

Hmm...

Never second guess a quantity you can calculate explicitly.

Ismail, does this fix your occurrence of the problem, too?

diff --git a/Src/signals.c b/Src/signals.c
index 697c4c5..78dc75b 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -528,8 +528,14 @@ wait_for_processes(void)
 	 * and is not equal to the current foreground job.
 	 */
 	if (jn && !(jn->stat & (STAT_CURSH|STAT_BUILTIN)) &&
-	    jn - jobtab != thisjob)
-	    addbgstatus(pid, (int)lastval2);
+	    jn - jobtab != thisjob) {
+	    int val = (WIFSIGNALED(status) ?
+		   0200 | WTERMSIG(status) :
+		   (WIFSTOPPED(status) ?
+		    0200 | WEXITSTATUS(status) :
+		    WEXITSTATUS(status)));
+	    addbgstatus(pid, val);
+	}
     }
 }
 

A05 test failure (was Re: 5.0.9 eventually...?)

[Bart Schaefer]

On Aug 10,  7:12am, Ismail Donmez wrote:
}
} latest git started to fail here on Linux x64:
} 
} ./A05execution.ztst: starting.
} This test takes 5 seconds to fail...
} Test ./A05execution.ztst failed: bad status 2, expected 1 from:
} Was testing: The status of recently exited background jobs is recorded

This apparently has always been a race condition and queuing of signals
in more places simply made it obvious.  I can only reproduce the failed
test on one of the systems where I can try it.

What's happening is that the SIGCHLD for (exit 2) is arriving during the
run of zhandler() for recording the status of (exit 1).  The value of
the global lastval2 is not saved/restored around the signal handling
so when the (exit 1) handler resumes it copies the exit status of the
second child into the bgstatus list as the exit of the first child.

It's fairly easy to make any two of the jobs exchange status by adding
sleep or other delays.

Anyway, turns out this was an ANCIENT bug that the revised queueing
paradigm exposed.

diff --git a/Src/signals.c b/Src/signals.c
index 697c4c5..acdc0bc 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -487,6 +487,12 @@ wait_for_processes(void)
 	    break;
 	}
 
+	/* This is necessary to be sure queueing_enabled > 0 when
+	 * we enter printjob() from update_job(), so that we don't
+	 * decrement to zero in should_report_time() and improperly
+	 * run other handlers in the middle of processing this one */
+	queue_signals();
+
 	/*
 	 * Find the process and job containing this pid and
 	 * update it.
@@ -530,6 +536,8 @@ wait_for_processes(void)
 	if (jn && !(jn->stat & (STAT_CURSH|STAT_BUILTIN)) &&
 	    jn - jobtab != thisjob)
 	    addbgstatus(pid, (int)lastval2);
+
+	unqueue_signals();
     }
 }
 

-- 
Barton E. Schaefer

Re: Wait test failure

[Bart Schaefer]

On Aug 10,  4:48pm, Peter Stephenson wrote:
}
} Never second guess a quantity you can calculate explicitly.

This certainly avoids the problem with lastval2 that I described in
36079.  However, I think it's still wrong to run the queued signals
from inside should_report_time() when inside wait_for_processes() ?

Re: Wait test failure

[Peter Stephenson]

On Mon, 10 Aug 2015 10:44:32 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Aug 10,  4:48pm, Peter Stephenson wrote:
> }
> } Never second guess a quantity you can calculate explicitly.
> 
> This certainly avoids the problem with lastval2 that I described in
> 36079.  However, I think it's still wrong to run the queued signals
> from inside should_report_time() when inside wait_for_processes() ?

Yes, if this is anything to go by... it's a pretty obscure problem, too,
since I couldn't work out what was going on in this case.

pws

Re: A05 test failure (was Re: 5.0.9 eventually...?)

[Ismail Donmez]

Bart Schaefer writes:
 
> Anyway, turns out this was an ANCIENT bug that the revised queueing
> paradigm exposed.

Confirmed as fixed, thank you all!

Re: 5.0.9 eventually...?

[Peter Stephenson]

Are we nearly there yet...?

pws

Re: 5.0.9 eventually...?

[Bart Schaefer]

On Aug 17,  8:54pm, Peter Stephenson wrote:
}
} Are we nearly there yet...?

Let me back out that _expand change per Oliver, and then we're probably
as ready as we're going to be -- have to call a cutoff somewhere.

Re: 5.0.9 eventually...?

[Mikael Magnusson]

On Mon, Aug 17, 2015 at 9:54 PM, Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
> Are we nearly there yet...?
>
> pws

I just updated my tree and opened a shell, and nothing blew up. I
don't know how much more rigorous a QA check can be so we're good to
go on my part.

-- 
Mikael Magnusson