Opened 2 years ago

Closed 2 years ago

#28354 closed defect (fixed)

pexpect GAP interface: Handle errors when subprocess isn't wait()-ed by ptyprocess

Reported by: embray Owned by:
Priority: major Milestone: sage-8.9
Component: interfaces Keywords: gap pexpect
Cc: Merged in:
Authors: Erik Bray Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: 78f92f6 (Commits, GitHub, GitLab) Commit: 78f92f6678eb468764701c2386dbe7bc06d22562
Dependencies: Stopgaps:

Status badges

Description

As mentioned in #20178, pexpect used to leave zombie processes around upon EOF.

Now that appears to be fixed. Unfortunately, if the EOF occurs because the process was killed and wait()-ed by a different process, and not by pexpect itself, rather than just ignore the situation and set the process as closed, pexpect (really ptyprocess in this case) raises an exception and does not mark the process as terminated.

Therefore any subsequent attempt to restart the GAP process tries again to call isalive() and just gets the same exception again.

We should instead catch this exception and just force the process to be marked as terminated.

Change History (12)

comment:1 Changed 2 years ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/ticket-28354
  • Status changed from new to needs_review

This is a bit of a mess but it seems to take care of it.

Although #20178 appears to be "fixed" the fix is not so useful, because now any situation where isalive() gets called results in unhandled exceptions in the case described in this ticket, where the underling process died unexpectedly.

comment:2 Changed 2 years ago by vbraun

  • Status changed from needs_review to needs_work

No branch?

comment:3 Changed 2 years ago by git

  • Commit set to 358d626e4cdbaf19983f04bf6492c7ce1d44c218

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

358d626Trac #28354: Better handling of unhandled exceptions from pexpect when

comment:4 Changed 2 years ago by embray

  • Status changed from needs_work to needs_review

Didn't push, apparently.

comment:5 Changed 2 years ago by git

  • Commit changed from 358d626e4cdbaf19983f04bf6492c7ce1d44c218 to 200623eb276a2c7c9f20550883128b4465cb3edc

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

200623eTrac #28354: Better handling of unhandled exceptions from pexpect when

comment:6 Changed 2 years ago by embray

(fixed typo in a comment)

comment:7 Changed 2 years ago by vbraun

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

comment:8 Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work
File "src/sage/interfaces/gap3.py", line 568, in sage.interfaces.gap3.Gap3._install_hints
Failed example:
    gap3('3+2')
Expected:
    Traceback (most recent call last):
    ...
    TypeError: unable to start gap3 because the command '/wrongpath/gap3 ...' failed: The command was not found or was not executable: /wrongpath/gap3.
    <BLANKLINE>
        Your attempt to start GAP3 failed, either because you do not have
        have GAP3 installed, or because it is not configured correctly.
    <BLANKLINE>
        - If you do not have GAP3 installed, then you must either...
Got:
    <BLANKLINE>
    Traceback (most recent call last):
      File "/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 681, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1105, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.interfaces.gap3.Gap3._install_hints[1]>", line 1, in <module>
        gap3('3+2')
      File "/home/release/Sage/local/lib/python2.7/site-packages/sage/interfaces/interface.py", line 288, in __call__
        return cls(self, x, name=name)
      File "/home/release/Sage/local/lib/python2.7/site-packages/sage/interfaces/gap3.py", line 704, in __init__
        super(GAP3Element, self).__init__(parent, value, is_name, name)
      File "/home/release/Sage/local/lib/python2.7/site-packages/sage/interfaces/expect.py", line 1471, in __init__
        self._name = parent._create(value, name=name)
      File "/home/release/Sage/local/lib/python2.7/site-packages/sage/interfaces/interface.py", line 491, in _create
        self.set(name, value)
      File "/home/release/Sage/local/lib/python2.7/site-packages/sage/interfaces/interface.py", line 449, in set
        self.eval(cmd)
      File "/home/release/Sage/local/lib/python2.7/site-packages/sage/interfaces/gap.py", line 583, in eval
        result = Expect.eval(self, input_line, **kwds)
      File "/home/release/Sage/local/lib/python2.7/site-packages/sage/interfaces/expect.py", line 1385, in eval
        for L in code.split('\n') if L != ''])
      File "/home/release/Sage/local/lib/python2.7/site-packages/sage/interfaces/gap.py", line 786, in _eval_line
        if expect_eof:
    UnboundLocalError: local variable 'expect_eof' referenced before assignment

comment:9 Changed 2 years ago by git

  • Commit changed from 200623eb276a2c7c9f20550883128b4465cb3edc to f0c2d6eca5b1971f41da9a12303c699d59fc715b

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

f0c2d6eMove definition of expect_eof before anything that can raise an exception in the try/except

comment:10 Changed 2 years ago by git

  • Commit changed from f0c2d6eca5b1971f41da9a12303c699d59fc715b to 78f92f6678eb468764701c2386dbe7bc06d22562

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

78f92f6Trac #28354: Better handling of unhandled exceptions from pexpect when

comment:11 Changed 2 years ago by embray

  • Status changed from needs_work to positive_review

Fixed stupid coding error. Tested directly so I'll set back to positive_review if that was the only issue.

comment:12 Changed 2 years ago by vbraun

  • Branch changed from u/embray/ticket-28354 to 78f92f6678eb468764701c2386dbe7bc06d22562
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.