pexpect interfaces are never deleted

[alexc]

This crashes because the Dokchitser instances are kept alive all the time.

sage: for i in range(2000):
....:     D = Dokchitser(conductor=1, gammaV=[0], weight=1, eps=1, poles=[1], residues=[-1], init='1')

[jdemeyer]

Replying to alexc:

Let me know what additional information I can provide to help.

A reproducible example...

[alexc]

Example

[alexc]

Replying to jdemeyer:

Replying to alexc:

Let me know what additional information I can provide to help.

A reproducible example...

I've attached a program which produces the error. The program fails after count = 160 for me.

[jdemeyer]

New commits:

​8501bb1	Clean up in expect.py
​821151d	Implement proper kill-on-del for pexpect

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​d556229	Further clean-up of interfaces
​012c62c	Implement proper kill-on-del for pexpect

[rws]

With #17718 merged:

sage -t --long src/sage/interfaces/sagespawn.pyx  # 2 doctests failed
sage -t --long src/sage/interfaces/quit.py  # 1 doctest failed

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​c8d1a28	Revert interrupt change
​815fbc6	Merge commit 'c8d1a289e95184f3f6f5528e8fb8ee4638ab47c0' into t/17686/ticket/17686
​47646e9	Merge remote-tracking branch 'origin/develop' into t/17686/ticket/17686
​fede9b0	Fix doctest for GP

[jdemeyer]

Merged #17718 and latest develop. I also trivially fixed the quit.py failure but I don't get any failures in sagespawn.pyx. Can you try again and post the details of the failure if they still occur?

[rws]

Yes, quit.py is fine but I still get this twice:

File "src/sage/interfaces/sagespawn.pyx", line 69, in sage.interfaces.sagespawn.SageSpawn.__repr__
Failed example:
    s  # indirect doctest
Expected:
    stupid process with PID ... running /bin/true
Got:
    stupid process with PID 3742 running /usr/bin/true

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​76c99e9

Don't put full path to "true" in doctest

[rws]

Patchbot says tests are fine, whines about startup time, but on my machine there is no change. My Unix programming doesn't suffice for a code review, however.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​0b3c7f5

Merge tag '6.6.beta2' into t/17686/ticket/17686

[dimpase]

doesn't merge cleanly againts the latest 6.7.beta4:

CONFLICT (content): Merge conflict in src/doc/en/reference/interfaces/index.rst

can this be rebased?

[jdemeyer]

Replying to dimpase:

doesn't merge cleanly againts the latest 6.7.beta4:

CONFLICT (content): Merge conflict in src/doc/en/reference/interfaces/index.rst

can this be rebased?

Will it be reviewed then?

[dimpase]

Replying to jdemeyer:

Replying to dimpase:

doesn't merge cleanly againts the latest 6.7.beta4:

CONFLICT (content): Merge conflict in src/doc/en/reference/interfaces/index.rst

can this be rebased?

Will it be reviewed then?

I will do my best.

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​864d55a

Implement proper kill-on-del for pexpect

[malb]

Why is SageSpawn written in Cython? Reading over it, I couldn't find a reason, which probably just means I missed it.

[jdemeyer]

Replying to malb:

Why is SageSpawn written in Cython?

1. it uses various system calls (of course, you could argue about using Python's os module but I think directly using system calls is better).
2. it needs Py_INCREF() which cannot be done in pure Python.

[malb]

Thanks for explaining, that makes sense.

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​4810d72

Implement proper kill-on-del for pexpect

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​08e422a

Implement proper kill-on-del for pexpect

[vbraun]

The original spawn class checks self.closed in the dtor:

    def __del__(self):
        if self.closed:
            return
        try:
            self.close()
        except:
            pass

So why not just set that flag in _keep_alive? Or is there another resource managed by an attribute with a non-trivial dtor? This seems better than keeping the spawn instance alive forever... maybe I'm missing something?

[jdemeyer]

Replying to vbraun:

So why not just set that flag in _keep_alive? Or is there another resource managed by an attribute with a non-trivial dtor? This seems better than keeping the spawn instance alive forever... maybe I'm missing something?

Py_INCREF(self) makes no assumptions on how pexpect.spawn works internally. Setting self.closed would work currently, but it would for example break with a newer version of pexpect.

[vbraun]

Fails on OSX:

sage -t --long src/sage/interfaces/expect.py  # 26 doctests failed
sage -t --long src/sage/interfaces/gap.py  # 95 doctests failed
sage -t --long src/sage/interfaces/mwrank.py  # 6 doctests failed

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​15e42fe

Wrap sendline() in try/except

[vbraun]

I got this (with #17924, only randomly fails)

sage -t --long src/sage/interfaces/sagespawn.pyx
**********************************************************************
File "src/sage/interfaces/sagespawn.pyx", line 125, in sage.interfaces.sagespawn.SageSpawn.close
Failed example:
    while s.isalive():  # long time (5 seconds)
        sleep(0.1)
Exception raised:
    Traceback (most recent call last):
      File "/home/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 496, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 858, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.interfaces.sagespawn.SageSpawn.close[3]>", line 1, in <module>
        while s.isalive():  # long time (5 seconds)
      File "/home/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/pexpect.py", line 762, in isalive
        pid, status = os.waitpid(self.pid, waitpid_options)
      File "sage/ext/interrupt/interrupt.pyx", line 197, in sage.ext.interrupt.interrupt.sage_python_check_interrupt (/home/buildslave-sage/slave/sage_git/build/src/build/cythonized/sage/ext/interrupt/interrupt.c:1743)
        sig_check()
      File "sage/ext/interrupt/interrupt.pyx", line 86, in sage.ext.interrupt.interrupt.sig_raise_exception (/home/buildslave-sage/slave/sage_git/build/src/build/cythonized/sage/ext/interrupt/interrupt.c:884)
        raise KeyboardInterrupt
    KeyboardInterrupt
**********************************************************************
1 item had failures:
   1 of   5 in sage.interfaces.sagespawn.SageSpawn.close
    [24 tests, 1 failure, 5.08 s]

[jdemeyer]

Replying to vbraun:

I got this (with #17924, only randomly fails)

On which machine?

This is probably yet another race condition: it looks like we are killing the process group of the child process when it has not yet created a new process group. So, we end up killing ourselves.

[vbraun]

That was on arando. I'm going to close this ticket and leave the remaining race to you, then ;-)

[vbraun]

Followup at #18741