#17686 closed defect (fixed)
pexpect interfaces are never deleted
Reported by:  alexc  Owned by:  

Priority:  major  Milestone:  sage6.6 
Component:  interfaces  Keywords:  
Cc:  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Volker Braun 
Report Upstream:  N/A  Work issues:  
Branch:  15e42fe (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
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')
Attachments (1)
Change History (39)
comment:1 in reply to: ↑ description ; followup: ↓ 2 Changed 8 years ago by
comment:2 in reply to: ↑ 1 Changed 8 years ago by
comment:3 Changed 8 years ago by
comment:4 Changed 8 years ago by
 Component changed from number theory to interfaces
 Description modified (diff)
 Summary changed from computel.gp: "unable to start pari because the command 'gp emacs quiet stacksize 10000000' failed" to pexpect interfaces don't close logfile
comment:5 Changed 8 years ago by
 Description modified (diff)
 Summary changed from pexpect interfaces don't close logfile to pexpect interfaces are never deleted
comment:6 Changed 8 years ago by
 Dependencies set to #17704
comment:7 Changed 8 years ago by
 Branch set to u/jdemeyer/ticket/17686
 Commit set to 821151de14a55ccac635eaef4dadf861927a7df4
 Status changed from new to needs_review
comment:8 Changed 8 years ago by
 Commit changed from 821151de14a55ccac635eaef4dadf861927a7df4 to 012c62ce2cb9b5301859d083722d0ee01321bb64
comment:9 Changed 8 years ago by
 Dependencies changed from #17704 to #17704, #17718
comment:10 Changed 7 years ago by
 Milestone changed from sage6.5 to sage6.6
 Status changed from needs_review to needs_work
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
comment:11 Changed 7 years ago by
 Commit changed from 012c62ce2cb9b5301859d083722d0ee01321bb64 to fede9b02e6f6869461f147bef569a791394ec6b3
comment:12 Changed 7 years ago by
 Status changed from needs_work to needs_review
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?
comment:13 Changed 7 years ago by
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
comment:14 Changed 7 years ago by
 Commit changed from fede9b02e6f6869461f147bef569a791394ec6b3 to 76c99e99410d2f43d1c3bfbcbf6d44c09510d20d
Branch pushed to git repo; I updated commit sha1. New commits:
76c99e9  Don't put full path to "true" in doctest

comment:15 Changed 7 years ago by
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.
comment:16 Changed 7 years ago by
 Commit changed from 76c99e99410d2f43d1c3bfbcbf6d44c09510d20d to 0b3c7f55cc79f9654ffc62058ebda120f5db5b2d
Branch pushed to git repo; I updated commit sha1. New commits:
0b3c7f5  Merge tag '6.6.beta2' into t/17686/ticket/17686

comment:17 followup: ↓ 18 Changed 7 years ago by
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?
comment:18 in reply to: ↑ 17 ; followup: ↓ 19 Changed 7 years ago by
Replying to dimpase:
doesn't merge cleanly againts the latest 6.7.beta4:
CONFLICT (content): Merge conflict in src/doc/en/reference/interfaces/index.rstcan this be rebased?
Will it be reviewed then?
comment:19 in reply to: ↑ 18 Changed 7 years ago by
comment:20 Changed 7 years ago by
 Commit changed from 0b3c7f55cc79f9654ffc62058ebda120f5db5b2d to 864d55aca034b59c2b3e2817ff5c1394dbed5658
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
864d55a  Implement proper killondel for pexpect

comment:21 followup: ↓ 22 Changed 7 years ago by
Why is SageSpawn
written in Cython? Reading over it, I couldn't find a reason, which probably just means I missed it.
comment:22 in reply to: ↑ 21 Changed 7 years ago by
Replying to malb:
Why is
SageSpawn
written in Cython?
 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).  it needs
Py_INCREF()
which cannot be done in pure Python.
comment:23 Changed 7 years ago by
Thanks for explaining, that makes sense.
comment:24 Changed 7 years ago by
 Commit changed from 864d55aca034b59c2b3e2817ff5c1394dbed5658 to 4810d72d22055cd2655f6e559e9fbdbe4d6b470e
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4810d72  Implement proper killondel for pexpect

comment:25 Changed 7 years ago by
 Dependencies #17704, #17718 deleted
comment:26 Changed 7 years ago by
 Commit changed from 4810d72d22055cd2655f6e559e9fbdbe4d6b470e to 08e422aaabf904356b4d53e68cf30f033a57585e
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
08e422a  Implement proper killondel for pexpect

comment:27 followup: ↓ 28 Changed 7 years ago by
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 nontrivial dtor? This seems better than keeping the spawn instance alive forever... maybe I'm missing something?
comment:28 in reply to: ↑ 27 Changed 7 years ago by
Replying to vbraun:
So why not just set that flag in
_keep_alive
? Or is there another resource managed by an attribute with a nontrivial 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
.
comment:29 Changed 7 years ago by
 Reviewers set to Volker Braun
 Status changed from needs_review to positive_review
comment:30 Changed 7 years ago by
 Status changed from positive_review to needs_work
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
comment:31 Changed 7 years ago by
 Commit changed from 08e422aaabf904356b4d53e68cf30f033a57585e to 15e42feddfb154b9bf229b6bccf43d4604158dbe
Branch pushed to git repo; I updated commit sha1. New commits:
15e42fe  Wrap sendline() in try/except

comment:32 Changed 7 years ago by
 Status changed from needs_work to needs_review
comment:33 Changed 7 years ago by
 Status changed from needs_review to positive_review
comment:34 followup: ↓ 35 Changed 7 years ago by
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/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 496, in _run self.compile_and_execute(example, compiler, test.globs) File "/home/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/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/buildslavesage/slave/sage_git/build/local/lib/python2.7/sitepackages/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/buildslavesage/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/buildslavesage/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]
comment:35 in reply to: ↑ 34 Changed 7 years ago by
comment:36 Changed 7 years ago by
That was on arando. I'm going to close this ticket and leave the remaining race to you, then ;)
comment:37 Changed 7 years ago by
 Branch changed from u/jdemeyer/ticket/17686 to 15e42feddfb154b9bf229b6bccf43d4604158dbe
 Resolution set to fixed
 Status changed from positive_review to closed
comment:38 Changed 7 years ago by
 Commit 15e42feddfb154b9bf229b6bccf43d4604158dbe deleted
Followup at #18741
Replying to alexc:
A reproducible example...