Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent exponential number of calls while accessing "closed" / "isatty" (#196) #198

Merged
merged 1 commit into from
Nov 3, 2018

Conversation

Delgan
Copy link
Contributor

@Delgan Delgan commented Oct 15, 2018

See #197 for extended discussion.

@bbayles
Copy link

bbayles commented Oct 16, 2018

This does fix the problem in #196.

@wiggin15
Copy link
Collaborator

@Delgan I think I understand your comment in #197 about the call traces being more complex than needed, but I don't think I follow what the problem in #196 is exactly. If we wrap the streams too many times then this fix will only be a temporary relief and not a real fix, because there will still be too many nesting calls. Won't there..? If I understand correctly the problem was not maximum recursion?

It will help to understand the problem if you can provide a minimal sample code (without modifying colorama) that reproduces this and explain exactly what happens. Thanks.

@Delgan
Copy link
Contributor Author

Delgan commented Oct 21, 2018

@wiggin15 You seem to have very well understood the problem. :)

The following program should hang forever (make sure to run it on Windows, or add autoreset=True to ensure that sys.stderr and sys.stdout are continously re-wrapped).

import colorama

for i in range(100):
    colorama.init()

You can observe how the time required to retrieve .closed increases as the stream is consecutively wrapped (calling init() multiple times has a similar behavior):

import time
import sys
from colorama import AnsiToWin32

stream = sys.stderr

while True:
    stream = AnsiToWin32(stream, convert=True).stream
    start = time.process_time()
    stream.closed
    end = time.process_time()
    print(end - start)

This is not a proper "nesting" issue, the issue is that fetching .closed on the wrapper stream requires two .closed accesses on the wrapped stream. So, if you wrap the same stream 100 times, you will need 1 + 2 + 4 + 8 + ... + 2^100 = 2.5353012e+30 functions calls. With the fix, it's only 1 + 1 + 1 ... + 1 = 100.

The maximum recursion depth can't even be reached, so there is no exception. This is the exponential number of functions calls to be made hich provokes the program to hangs when accessing .closed (see callstack of my other comment).

Running the same code with my changes will not freeze the program but instead raises a RecursionError: maximum recursion depth exceeded after a few milliseconds.

The issue #196 appears when calling init() too many times. This may happen in the unit tests of some code base, according to @bbayles. As calling init() several times causes sys.stderr and sys.stdout to be wrapped multiple times, the previously described behavior occurs.
With this PR, the threshold from which calling .init() too many times causes problems is much higher. It's a fix for #196 as it prevent early hanging, RecursionError could still happen in extreme cases, but this is another issue in my opinion (probably related to #140 and #182).

@wiggin15
Copy link
Collaborator

wiggin15 commented Nov 1, 2018

Thanks @Delgan . This is a fairly simple change and if it has this big an effect I will merge this. One question, though: if we revert the change in 2057f03 (make closed and isatty properties), will that make the potential RecursionError in this case go away?

@Delgan
Copy link
Contributor Author

Delgan commented Nov 1, 2018

@wiggin15 Unfortunately, reverting 2057f03 would not help with that. RecursionError are unavoidable when wrapping objects continuously, that's how Python works.
Without 2057f03, the exception would be raised by __getattr__() or write() as demonstrated by the previously linked issues. Accessing wrapper attribute requires accessing the deepest wrapped object, hence the error.

The most straightforward workaround is to not wrap the same stream too many times (what's the point anyway?).
Otherwise, maybe that one can think of a new implementation which does not use recursion, but that requires to totally refactor AnsiToWin32.

@wiggin15 wiggin15 merged commit 90f2fac into tartley:master Nov 3, 2018
@wiggin15
Copy link
Collaborator

wiggin15 commented Nov 3, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants