Heavy use of CallWithTimeout makes GAP crash / produce wrong results

[Stefan-Kohl]

In GAP 4.8.3, in sessions with heavy use of CallWithTimeout I observed the following:

• crash with segfault when hitting [ctrl][c], instead of entering a break loop
• unpredictable wrong results, sometimes not obvious

These seem to happen only when making heavy use of CallWithTimeout, but I am
not sure under which precise circumstances they do. My guess is that sometimes
things are left in an inconsistent state when a timeout occurs.

[ChrisJefferson]

Crashes we should fix, if we can figure out how to reproduce them.

Unpredictable wrong results are, unfortunately, always going to happen, and are I think may be an unfixable problem. I realise that's a scary-sounding thing to say!

[markuspf]

What code are you running to produce this behaviour?

I have observed similar things last night but didn't get to investigate yet. I
think there might be a problem with signal delivery/handling.

[markuspf]

Running the current grplatt.tst test, waiting, and then hitting Ctrl-C crashes my GAP.

[Stefan-Kohl]

For me, this occurred in a longer GAP session which is not easily reproducible.

[markuspf]

testing: /home/makx/ac/gap/tst/teststandard/small_groups2.tst
^CAssertion failed: (NumAlarmJumpBuffers), function CheckAndRespondToAlarm, file ../../src/stats.c, line 1641.

Looks like SIGALARM is delivered (or some other way CheckAndRespondToAlarm is called). Since this happens with Ctrl-C being pressed, I'll have a look at the signal handling code quickly.

[Stefan-Kohl]

Yes, this is the message I also get when GAP crashes. A way to reproduce the crash is to run the
following code for a while and then press [ctrl][c]:

    G := SymmetricGroup(20); i := 1; aborted := 0;
    repeat
      if i mod 1000 = 0 then Print("i = ",i,", aborted = ",aborted,"\n"); fi;
      a := Random(G); b := Random(G);
      c := Random(Group(a,b));
      if CallWithTimeout(5,\in,c,Group(a,b)) = [false] then aborted := aborted + 1; fi;
      error := not c in Group(a,b);
      i := i + 1;
    until error;

I don't know whether the error is triggered eventually if one waits long enough.

[ChrisJefferson]

I've found one issue -- on stats.c:164, we should set SyAlarmHasGoneOff to 0 straight away, else we can head through this twice -- or (perhaps easier, but might hide other bugs) if we end up here with no alarm buffers just return rather than asserting.

[stevelinton]

The documentation does say

If   the   call  does  not  complete  within  the  timeout,  the  result  of
CallWithTimeout[List]  is  a  list of length 1 containing the value false In
this  case,  just  as  if you had quit from a break loop, there is some risk
that  internal  data structures in GAP may have been left in an inconsistent
state, and you should proceed with caution.

[ChrisJefferson]

The segfaulting is (hopefully) fixed in #698. Is actually a very small fix.

This does introduce a different (tiny) bug -- if you press ctrl+c just as a timer goes off, the ctrl+c will get ignored. I'm just going to ignore that, because fixing it would be (I think) reasonably fiddly, and that is a much less serious issue to fix, particularly in stable.

[fingolfin]

See #664 for a suggestion to phase out CallWithTimeout again due to major issues with it.

[stevelinton]

That’s a pity. It is no more dangerous than running the code and hitting ^C and then quit; which has always carried the risk of leaving your workspace in an inconsistent state, but I guess if you make it a programmable people will use it without being aware of it. Steve

…

On 16 Mar 2017, at 23:10, Max Horn ***@***.***> wrote: See #664 for a suggestion to phase out CallWithTimeout again due to major issues with it. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread. {"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/gap-system/gap","title":"gap-system/gap","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in ***@***.*** in #695: See #664 for a suggestion to phase out `CallWithTimeout` again due to major issues with it."}],"action":{"name":"View Issue","url":"#695 (comment)"}}}

[fingolfin]

@Stefan-Kohl Have you tried using ParTakeFirstResultByFork from the io package for this purpose instead?

[ChrisJefferson]

@Stefan-Kohl : But you realise, any answers you calculate when using CallWithTimeout can simply be wrong? And this is a real problem, I've had it happen. If you are using it, and care about the results, that is very bad.

[markuspf]

As noted before, this is fundamentally hard to do, and the only way of doing this without completely rewriting GAP, is to use fork to take a snapshot of the state of the GAP process before you start a computation, so that you get to a well-defined state at the end.

[Stefan-Kohl]

@markuspf What does "completely rewrite GAP" mean here? -- I would assume this concerns only the kernel, thus only a tiny part of the GAP system(?)

[ChrisJefferson]

@Stefan-Kohl : Could you try this replacement CallWithTimeout, and see if it works for you? At the moment, timeout must always be given in seconds (this is a bit different to CallWithTimeout, but is just because it was fast to do to start). Also your function must return something (with CallWithTimeout it can not return. These two issues can be fixed if this basic idea is an acceptable substitute.

IO_CallWithTimeout := function ( timeout, func, args... )
    local  ret;
    ret := ParDoByFork( [ func ], [ args ], rec(
          TimeOut := rec(
              tv_sec := timeout,
              tv_usec := 0 ) ) );
    if ret = [  ]  then
        return [ false ];
    else
        return [ true, ret[1] ];
    fi;
    return;
end;

[Stefan-Kohl]

@ChrisJefferson This seems to work -- thank you!

[ChrisJefferson]

@Stefan-Kohl : If you start using that, please rename the function. Just because I plan to add that function to IO, probably with that name, and wouldn't want your code to break when the function is officially added to the IO package!

[fingolfin]

Resolved by #1324.