Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pexpect interfaces are never deleted #17686

Closed
sagetrac-alexc mannequin opened this issue Jan 29, 2015 · 48 comments
Closed

pexpect interfaces are never deleted #17686

sagetrac-alexc mannequin opened this issue Jan 29, 2015 · 48 comments

Comments

@sagetrac-alexc
Copy link
Mannequin

sagetrac-alexc mannequin commented Jan 29, 2015

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')

Component: interfaces

Author: Jeroen Demeyer

Branch: 15e42fe

Reviewer: Volker Braun

Issue created by migration from https://trac.sagemath.org/ticket/17686

@sagetrac-alexc sagetrac-alexc mannequin added this to the sage-6.5 milestone Jan 29, 2015
@jdemeyer
Copy link

Replying to @sagetrac-alexc:

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

A reproducible example...

@sagetrac-alexc
Copy link
Mannequin Author

sagetrac-alexc mannequin commented Jan 30, 2015

Attachment: computeltest.py.gz

Example

@sagetrac-alexc
Copy link
Mannequin Author

sagetrac-alexc mannequin commented Jan 30, 2015

comment:2

Replying to @jdemeyer:

Replying to @sagetrac-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

This comment has been minimized.

@jdemeyer jdemeyer changed the title computel.gp: "unable to start pari because the command 'gp --emacs --quiet --stacksize 10000000' failed" pexpect interfaces don't close logfile Jan 30, 2015
@jdemeyer

This comment has been minimized.

@jdemeyer jdemeyer changed the title pexpect interfaces don't close logfile pexpect interfaces are never deleted Jan 31, 2015
@jdemeyer
Copy link

Dependencies: #17704

@jdemeyer
Copy link

jdemeyer commented Feb 2, 2015

New commits:

8501bb1Clean up in expect.py
821151dImplement proper kill-on-del for pexpect

@jdemeyer
Copy link

jdemeyer commented Feb 2, 2015

Branch: u/jdemeyer/ticket/17686

@jdemeyer
Copy link

jdemeyer commented Feb 2, 2015

Author: Jeroen Demeyer

@jdemeyer
Copy link

jdemeyer commented Feb 2, 2015

Commit: 821151d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 3, 2015

Changed commit from 821151d to 012c62c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 3, 2015

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

d556229Further clean-up of interfaces
012c62cImplement proper kill-on-del for pexpect

@jdemeyer
Copy link

jdemeyer commented Feb 3, 2015

Changed dependencies from #17704 to #17704, #17718

@rwst
Copy link

rwst commented Feb 25, 2015

comment:10

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

@rwst rwst modified the milestones: sage-6.5, sage-6.6 Feb 25, 2015
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 25, 2015

Changed commit from 012c62c to fede9b0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 25, 2015

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

c8d1a28Revert interrupt change
815fbc6Merge commit 'c8d1a289e95184f3f6f5528e8fb8ee4638ab47c0' into t/17686/ticket/17686
47646e9Merge remote-tracking branch 'origin/develop' into t/17686/ticket/17686
fede9b0Fix doctest for GP

@jdemeyer
Copy link

comment:12

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?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 27, 2015

Changed commit from 864d55a to 4810d72

@jdemeyer
Copy link

Changed dependencies from #17704, #17718 to none

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 29, 2015

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

08e422aImplement proper kill-on-del for pexpect

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 29, 2015

Changed commit from 4810d72 to 08e422a

@vbraun
Copy link
Member

vbraun commented Jun 17, 2015

comment:27

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
Copy link

comment:28

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
Copy link
Member

vbraun commented Jun 17, 2015

Reviewer: Volker Braun

@vbraun
Copy link
Member

vbraun commented Jun 17, 2015

comment:30

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 18, 2015

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

15e42feWrap sendline() in try/except

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 18, 2015

Changed commit from 08e422a to 15e42fe

@vbraun
Copy link
Member

vbraun commented Jun 18, 2015

comment:34

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
Copy link

comment:35

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
Copy link
Member

vbraun commented Jun 19, 2015

comment:36

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

@vbraun
Copy link
Member

vbraun commented Jun 19, 2015

Changed branch from u/jdemeyer/ticket/17686 to 15e42fe

@vbraun
Copy link
Member

vbraun commented Jun 19, 2015

Changed commit from 15e42fe to none

@vbraun
Copy link
Member

vbraun commented Jun 19, 2015

comment:38

Followup at #18741

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants