jquast / blessed

Blessed is an easy, practical library for making python terminal apps
http://pypi.python.org/pypi/blessed
MIT License
1.2k stars 72 forks source link

`get_keyboard_codes()` can throw a `RuntimeError` if a global variable is created while it is running #248

Closed adamnovak closed 1 year ago

adamnovak commented 1 year ago

In the get_keyboard_codes() function, blessed iterates over globals().items():

https://github.com/jquast/blessed/blob/1.19.1/blessed/keyboard.py#L114

If another Python thread exists, it can create (or destroy?) a global variable while Blessed is iterating. This can cause an exception, like this one that occurred in my project:

Traceback (most recent call last):
  File "/builds/databiosphere/toil/src/toil/test/sort/sortTest.py", line 217, in testFileSingleNonCaching
    self._toilSort(jobStoreLocator=self._getTestJobStorePath(), batchSystem='single_machine',
  File "/builds/databiosphere/toil/src/toil/test/sort/sortTest.py", line 164, in _toilSort
    with runMain(options):
  File "/usr/lib/python3.9/contextlib.py", line 119, in __enter__
    return next(self.gen)
  File "/builds/databiosphere/toil/src/toil/test/sort/sortTest.py", line 59, in runMain
    main(options)
  File "/builds/databiosphere/toil/src/toil/test/sort/sort.py", line 254, in main
    sortedFileID = workflow.restart()
  File "/builds/databiosphere/toil/src/toil/common.py", line 1065, in restart
    return self._runMainLoop(rootJobDescription)
  File "/builds/databiosphere/toil/src/toil/common.py", line 1468, in _runMainLoop
    return Leader(config=self.config,
  File "/builds/databiosphere/toil/src/toil/leader.py", line 269, in run
    with enlighten.get_manager(stream=sys.stderr, enabled=not self.config.disableProgress) as manager:
  File "/builds/databiosphere/toil/venv/lib/python3.9/site-packages/enlighten/manager.py", line 55, in get_manager
    return Manager(stream=stream, counter_class=counter_class, **kwargs)
  File "/builds/databiosphere/toil/venv/lib/python3.9/site-packages/enlighten/_manager.py", line 69, in __init__
    super(Manager, self).__init__(**kwargs)
  File "/builds/databiosphere/toil/venv/lib/python3.9/site-packages/enlighten/_basemanager.py", line 74, in __init__
    self.term = Terminal(stream=self.stream, kind=kind, force_styling=bool(kind))
  File "/builds/databiosphere/toil/venv/lib/python3.9/site-packages/blessed/terminal.py", line 208, in __init__
    self.__init__keycodes()
  File "/builds/databiosphere/toil/venv/lib/python3.9/site-packages/blessed/terminal.py", line 312, in __init__keycodes
    self._keycodes = get_keyboard_codes()
  File "/builds/databiosphere/toil/venv/lib/python3.9/site-packages/blessed/keyboard.py", line 114, in get_keyboard_codes
    keycodes.update((name, value) for name, value in globals().items() if name.startswith('KEY_'))
  File "/builds/databiosphere/toil/venv/lib/python3.9/site-packages/blessed/keyboard.py", line 114, in <genexpr>
    keycodes.update((name, value) for name, value in globals().items() if name.startswith('KEY_'))
RuntimeError: dictionary changed size during iteration

Either Blessed needs to make a snapshot of globals() and then loop over it, or get_keyboard_codes() needs to catch this error and retry its loop over globals().items() repeatedly until it gets through without a concurrent modification from another thread.

avylove commented 1 year ago

I wonder if globals().copy().items() would be sufficient. I would think the GIL would block during the copy operation.

avylove commented 1 year ago

@adamnovak I couldn't find a quick way to replicate the behavior to test. Can you see if adding copy() resolves it for your usecase?

adamnovak commented 1 year ago

I don't have a real reproduction for this; I've only ever seen it once and a reliable reproduction would involve writing a bunch of code to make a bunch of globals in a bunch of threads.

I think adding a copy() call would be the right way to address the issue; the dict documentation doesn't say that copy() can raise exceptions, or that it internally does any detectable "iteration".

avylove commented 1 year ago

Included copy() in 1.20.0. Hopefully that takes care of it, but please let us know if you see any more issues.