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 user from wrapping already wrapped streams #201

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tomchy
Copy link

@tomchy tomchy commented Oct 23, 2018

Due to the fact, that colorama uses global streams, multiple wraps around the same stream is pointless and causes python to hung on both Linux and Windows-based machines.

If such behavior is allowed, huge delays inside modules, that uses colorama are introduced:

>>> import colorama
>>> import time
>>> for i in range(30):
...   start = time.time()
...   colorama.init()
...   stop = time.time()
...   print(stop - start)
...
0.002007007598876953
0.004018068313598633
0.003504037857055664
0.0030028820037841797
0.004488945007324219
0.005014896392822266
0.0025060176849365234
0.007011890411376953
0.00401616096496582
0.01253199577331543
0.013036966323852539
0.03408479690551758
0.03659701347351074
0.06968498229980469
0.13085198402404785
0.22510790824890137
0.6968679428100586
1.0372772216796875
1.97328519821167
3.6422548294067383
4.66950798034668
12.083894968032837
25.121819019317627
51.497376918792725
104.18486189842224
177.13460302352905
272.79527592658997
652.5946300029755
...

Related issues:

@tomchy tomchy force-pushed the bugfix/prevent_from_rewraping_stdstream branch from ad2a750 to 2c395f8 Compare October 23, 2018 11:24
@Delgan
Copy link
Contributor

Delgan commented Oct 29, 2018

What if user wants to init() colorama with autoreset=True while the stream has already been wrapped by a library with autoreset=False?
It looks like this implementation is going to prevent him from getting the desired result, right?

The hanging and huge delays while wrapping the same stream is a big issue indeed, it has been reported in #196 and I opened #198 to fix it.

Adding a safeguard arround multiple wrapping could be useful though to avoid RecursionError when init() is called thousands of times. However, if that happens, I think the user is doing something wrong.

@tomchy tomchy force-pushed the bugfix/prevent_from_rewraping_stdstream branch from 2c395f8 to 9bbe4e8 Compare November 6, 2018 17:55
@tomchy
Copy link
Author

tomchy commented Nov 6, 2018

Hi Delgan!
Thank you for your quick response. You're right, I missed that case. There is even unit test that verifies the behaviour that you've mentioned.

This also brings another thought: if there is a test case that does a doubled initialisation, it may cause people to think that colorama may be initialised several times and the only effect should be the stream wrapper settings update. That leads to a conclusion: why calling init() thousands of times is something wrong, if a similar behaviour is presented inside unit tests?

That's also the reason why I've changed that unit test in the initial version, to avoid any further confusion in the usage of init() function.

Let me add a short story that shows how you may get into troubles with several init() calls:

  1. A developer tries to make a fancy logging module, that formats each log message in a way that errors are printed in red, warnings in yellow, and infos in white.
  2. Everything works perfectly on Linux, but one day he tries to use it under Windows. A well-proven solution for that is to use a mature project like colorama. The init() is called inside logger constructor, as that seems to be a logical place for it.
  3. After couple of months, the module starts to be used as a primary logging method.
  4. While project evolves, more instances of fancy logger are created, and one day - BOOM, everything starts to slow down, and finally - the software reaches RecursionError.

Of course, you may say - if someone does module-level initialisation at non-singleton object level, he has a bug in his code, but I bet that pasting one of unit tests would give you an approval in most of code reviews 🙂

@Delgan
Copy link
Contributor

Delgan commented Nov 8, 2018

Yeah, you are probably right and your example is pretty convincible. 🙂

The fact that several people have reported encountering RecursionError problems is also a hint that something could be improved.

I can't think of any downside with your suggested change, but it should be interesting to know why it has not been implemented like this in the first time and why nested wrapping had been chosen instead.

The only surprising effect would be in this case:

  1. Someone call init()
  2. Someone wrap or replace sys.stderr with another stream-object
  3. Someone call init() again

Then, the resulting sys.stderr would not be the one expected, but I don't know if such situation happens that often.

@tartley
Copy link
Owner

tartley commented Oct 13, 2020

Hey. FYI, Yesterday I created a PR to test releases before we push them to PyPI. When that is merged, I'll be more confident about resuming merges and releases. I'll try to look at this PR soon. Thank you for creating it!

@tartley
Copy link
Owner

tartley commented Oct 7, 2021

This PR dupes fixes in #236 and #262 (although this one predates those)

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

Successfully merging this pull request may close these issues.

3 participants