Opened 7 years ago

Closed 6 years ago

#3269 defect closed fixed (fixed)

curses.setupterm must only be called once per process

Reported by: therve Owned by: therve
Priority: normal Milestone:
Component: trial Keywords:
Cc: Branch: branches/curses-setupterm-3269
(diff, github, buildbot, log)
Author: therve Launchpad Bug:

Description

The class _AnsiColorizer in twisted.trial.reporter calls it for each supported() call. But this function seems to leak memory, so it should only be called if necessary. Unfortunately, there is no way to check if it has been called expect by trying to call a curses function needing it.

Change History (11)

comment:1 Changed 7 years ago by therve

  • author set to therve
  • Branch set to branches/curses-setupterm-3269

(In [23812]) Branching to 'curses-setupterm-3269'

comment:2 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

Please review.

comment:3 Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

os.environ is really terrible. I think the tests need to do something slightly different with respect to it. Consider:

>>> import os
>>> os.system('echo $LESSOPEN')
| /usr/bin/lesspipe %s
0
>>> os.environ.pop('LESSOPEN')
'| /usr/bin/lesspipe %s'
>>> os.system('echo $LESSOPEN')
| /usr/bin/lesspipe %s
0
>>> os.environ['LESSOPEN'] = 'hello, world'
>>> os.system('echo $LESSOPEN')
hello, world
0
>>> os.environ = {'LESSOPEN': 'foo bar'}
>>> os.system('echo $LESSOPEN')
hello, world
0
>>>

Also, in setUp, I think you probably meant to save os.environ.copy() instead of just os.environ, but I don't think that helps at all.

I feel like _AnsiColorizer should be using a terminfo database instead of curses (or it should be using curses to do the coloring as well, not producing color codes on its own). That may be a different ticket, though. I don't know if there's even a good way to read terminfo from Python, anyway.

comment:4 Changed 7 years ago by therve

(In [23847]) Remove cygwin related code.

Refs #3269

comment:5 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

I removed the code related to cygwin. This is not a supported platform, and I have no idea on how that works (I tried using cygwin with a win32 python installed, and that worked flawlessly). Hopefully someone interested later could add tests for that.

Please review.

comment:6 Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

Changes look good. I noticed a pre-existing behavior which may be a defect. _AnsiColorizer checks sys.stderr.isatty() but the reporter writes to sys.stdout. This means trial foo > bar will probably write colors to bar. I doubt this is intentional or desirable. I rather suspect that supported should accept the file-like object which is going to be written to (__init__ already does) and make the check against that, instead.

I'd like to take another look if you fix the stdout/stderr issue in this branch. If you'd rather file a separate ticket for that, cool and go ahead and merge this.

comment:7 Changed 6 years ago by therve

  • Keywords review added
  • Owner therve deleted

Back to review.

comment:8 Changed 6 years ago by exarkun

  • Keywords review removed
  • Owner set to therve
  • _Win32Colorizer still checks stdout, even if it is passed something else. I don't really care much about this. I don't even know how you'd get trial to be writing to something other than stdout on Windows. You could file another ticket for it, if you want.
  • twisted.trial.test.test_reporter.AnsiColorizerTests.test_supportedSetupTerm and twisted.trial.test.test_reporter.AnsiColorizerTests.test_supportedTigetNumErrors fail if run when stdout is not a pty.
  • MockColorizer.supported has a signature incompatible with the other colorizers. Since nothing fails because of this, I suppose that means it is never called, so maybe we don't need it

comment:9 Changed 6 years ago by therve

  • Keywords review added
  • Owner therve deleted

Points 2 and 3 addressed. I don't really care about 1 either.

comment:10 Changed 6 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

comment:11 Changed 6 years ago by therve

  • Resolution set to fixed
  • Status changed from new to closed

(In [24169]) Merge curses-setupterm-3269

Author: therve
Reviewer: exarkun
Fixes #3269

Fix trial AnsiColorizer so that it doesn't call curses.setupterm uselessly: it
caused a (small) memory leak in the reporter when using trial --until-failure.

Note: See TracTickets for help on using tickets.