Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#17686 closed defect (fixed)

pexpect interfaces are never deleted

Reported by: alexc Owned by:
Priority: major Milestone: sage-6.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:

Status badges

Description (last modified by jdemeyer)

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)

computeltest.py (1.7 KB) - added by alexc 7 years ago.
Example

Download all attachments as: .zip

Change History (39)

comment:1 in reply to: ↑ description ; follow-up: Changed 7 years ago by jdemeyer

Replying to alexc:

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

A reproducible example...

Changed 7 years ago by alexc

Example

comment:2 in reply to: ↑ 1 Changed 7 years ago by 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.

comment:3 Changed 7 years ago by jdemeyer

Last edited 7 years ago by jdemeyer (previous) (diff)

comment:4 Changed 7 years ago by jdemeyer

  • 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 7 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from pexpect interfaces don't close logfile to pexpect interfaces are never deleted

comment:6 Changed 7 years ago by jdemeyer

  • Dependencies set to #17704

comment:7 Changed 7 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Branch set to u/jdemeyer/ticket/17686
  • Commit set to 821151de14a55ccac635eaef4dadf861927a7df4
  • Status changed from new to needs_review

New commits:

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

comment:8 Changed 7 years ago by git

  • Commit changed from 821151de14a55ccac635eaef4dadf861927a7df4 to 012c62ce2cb9b5301859d083722d0ee01321bb64

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

comment:9 Changed 7 years ago by jdemeyer

  • Dependencies changed from #17704 to #17704, #17718

comment:10 Changed 7 years ago by rws

  • Milestone changed from sage-6.5 to sage-6.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 git

  • Commit changed from 012c62ce2cb9b5301859d083722d0ee01321bb64 to fede9b02e6f6869461f147bef569a791394ec6b3

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

comment:12 Changed 7 years ago by jdemeyer

  • 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 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

comment:14 Changed 7 years ago by git

  • Commit changed from fede9b02e6f6869461f147bef569a791394ec6b3 to 76c99e99410d2f43d1c3bfbcbf6d44c09510d20d

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

76c99e9Don't put full path to "true" in doctest

comment:15 Changed 7 years ago by 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.

comment:16 Changed 7 years ago by git

  • Commit changed from 76c99e99410d2f43d1c3bfbcbf6d44c09510d20d to 0b3c7f55cc79f9654ffc62058ebda120f5db5b2d

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

0b3c7f5Merge tag '6.6.beta2' into t/17686/ticket/17686

comment:17 follow-up: Changed 6 years ago by 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?

comment:18 in reply to: ↑ 17 ; follow-up: Changed 6 years ago by 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?

comment:19 in reply to: ↑ 18 Changed 6 years ago by 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.

comment:20 Changed 6 years ago by git

  • Commit changed from 0b3c7f55cc79f9654ffc62058ebda120f5db5b2d to 864d55aca034b59c2b3e2817ff5c1394dbed5658

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

864d55aImplement proper kill-on-del for pexpect

comment:21 follow-up: Changed 6 years ago by malb

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 6 years ago by 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.

comment:23 Changed 6 years ago by malb

Thanks for explaining, that makes sense.

comment:24 Changed 6 years ago by git

  • Commit changed from 864d55aca034b59c2b3e2817ff5c1394dbed5658 to 4810d72d22055cd2655f6e559e9fbdbe4d6b470e

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

4810d72Implement proper kill-on-del for pexpect

comment:25 Changed 6 years ago by jdemeyer

  • Dependencies #17704, #17718 deleted

comment:26 Changed 6 years ago by git

  • Commit changed from 4810d72d22055cd2655f6e559e9fbdbe4d6b470e to 08e422aaabf904356b4d53e68cf30f033a57585e

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

08e422aImplement proper kill-on-del for pexpect

comment:27 follow-up: Changed 6 years ago by 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?

comment:28 in reply to: ↑ 27 Changed 6 years ago by 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.

Last edited 6 years ago by jdemeyer (previous) (diff)

comment:29 Changed 6 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

comment:30 Changed 6 years ago by vbraun

  • 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 6 years ago by git

  • Commit changed from 08e422aaabf904356b4d53e68cf30f033a57585e to 15e42feddfb154b9bf229b6bccf43d4604158dbe

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

15e42feWrap sendline() in try/except

comment:32 Changed 6 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:33 Changed 6 years ago by vbraun

  • Status changed from needs_review to positive_review

comment:34 follow-up: Changed 6 years ago by 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]

comment:35 in reply to: ↑ 34 Changed 6 years ago by 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.

comment:36 Changed 6 years ago by vbraun

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

comment:37 Changed 6 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/17686 to 15e42feddfb154b9bf229b6bccf43d4604158dbe
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:38 Changed 6 years ago by vbraun

  • Commit 15e42feddfb154b9bf229b6bccf43d4604158dbe deleted

Followup at #18741

Note: See TracTickets for help on using tickets.