microsoft / knack

Knack - A Python command line interface framework
https://pypi.python.org/pypi/knack
MIT License
348 stars 95 forks source link

{Refactor} Remove unnecessary cli_ctx reference #213

Closed jiasli closed 3 years ago

jiasli commented 4 years ago

Description

Remove unnecessary cli_ctx reference, which makes pytest --capture=no fail:

RecursionError: maximum recursion depth exceeded

Root cause

When pytest captures stdout,

  1. pytest uses io.FileIO to patch sys.stdout
  2. When initializing CLI, CLI.out_file points to io.FileIO object
    https://github.com/microsoft/knack/blob/0d0308bc567f2d25654bb39201c02ab262d7ce0a/knack/cli.py#L33
  3. tests.util.redirect_io uses io.StringIO to patch sys.stdout, but this doesn't affect the existing CLI.out_file

Since CLI.out_file doesn't equal sys.__stdout__ (the original stdout), it won't be replaced with a colorama.ansitowin32.StreamWrapper:

https://github.com/microsoft/knack/blob/0d0308bc567f2d25654bb39201c02ab262d7ce0a/knack/cli.py#L195-L197

As PreviewItem (same for ExperimentalItem and Deprecated) keeps a reference to cli_ctx, during invocation

https://github.com/microsoft/knack/blob/fe3bf5d3a79a3dd2ce5ddb0c38d93843a3380f6f/knack/commands.py#L353

  1. PreviewItem is deepcopied ->
  2. cli_ctx is deepcopied ->
  3. CLI.out_file is deepcopied

Because io.FileIO can't be pickled, copy.deepcopy fails and reaches

https://github.com/microsoft/knack/blob/5f0bcc2ae416c8f2834db71b49040976e1d7a29d/knack/util.py#L78-L82

This makes cli_ctx not being deepcopied.

However, when pytest is called with pytest --capture=no, it doesn't patch sys.stdout,

  1. When initializing CLI, CLI.out_file equals sys.__stdout__
  2. tests.util.redirect_io uses io.StringIO to patch sys.stdout, but this doesn't affect the existing CLI.out_file

Since CLI.out_file equals sys.__stdout__ (the original stdout), it will be replaced with a colorama.ansitowin32.StreamWrapper and be deepcopied.

Because of a circular reference in AnsiToWin32

https://github.com/tartley/colorama/blob/3d8d48a95de10be25b161c914f274ec6c41d3129/colorama/ansitowin32.py#L28-L29

    def __getattr__(self, name):
        return getattr(self.__wrapped, name)

and io.StringIO can be pickled, when self.__wrapped is not set, deepcopy fails with exception RecursionError: maximum recursion depth exceeded.

To repro, use this simple script:

import colorama
import copy
import sys
import io

sys.stdout = io.StringIO()
colorama.init()
copy.deepcopy(sys.stdout)

Or

class StreamWrapper(object):
    def __init__(self, wrapped, converter):
        # self.__wrapped = wrapped
        self.__convertor = converter

    def __getattr__(self, name):
        return getattr(self.__wrapped, name)

sr = StreamWrapper("a", "b")
hasattr(sr, '__setstate__')