Fixed KeyError in canvas cleanup method

[markqvist]

Checklist

• I've ensured that similar functionality has not already been implemented
• I've ensured that similar functionality has not earlier been proposed and declined
• I've branched off the master or python-dual-support branch
• I've merged fresh upstream into my branch recently
• I've ran tox successfully in local environment

Description:

After long idle time of an app, the cleanup() method of canvas.py encounters a key-error when the canvas changes:

File "[...]/site-packages/urwid/canvas.py", Line 148, in cleanup del cls._refs[ref] KeyError: <weakref at 0x7fb6ce89a3421b, dead>

Wrapping in try will avoid exception being unhandled and dumped to UI.

[epurdy]

Can we please merge this PR? It's so simple and would make my app stop spewing warning messages for no reason.

[markqvist]

I would also really appreciate a merge, since I have a whole structure of strange IO redirection to avoid the user interface getting garbled by the error ;)

If there is anything that needs changing in my PR, or any other things that block merging, let me know, and I will be happy to help resolving it.

[ulidtko]

Hi @markqvist, thanks for the patch. 🙏

Any good way to reproduce the issue? I'll reference #280 too. What timescale do you mean: hours, days, weeks?..

Patch LGTM; though I'm suspicious there's a bug lurking deeper... Perhaps, what occasionally happens is that within 1 GC cycle a w: Widget gets rendered to a canvas instance, then invalidated (which calls CanvasCache.invalidate(w) and so clears CanvasCache._refs), then rendered again, creating a new (strongly-referenced) canvas instance. Only in that case I find it possible for CanvasCache.cleanup(a_ref) to get called with a_ref already not present in CanvasCache._refs but only just getting finalized.

Also, rewriting this using weakref.WeakKeyDictionary or even just weakref.finalize seems like a good idea, WDYT? It'll obviate most of the try-excepts and make the module simpler to understand.

[waveform80]

A (minor) suggestion: WeakKeyDictionary is certainly one option, though I suspect it would still leave a few cases of try..except KeyError lying around for those cases of retrieval from the weak dictionary where a key might've disappeared. Another alternative is to use dict.pop with a default (effectively ignoring retrieval errors), or my personal favourite contextlib.suppress (which is a bit less wordy than try..except but keeps the sense that a potential exception is being deliberately ignored), e.g.

# try..except
try:
    foo['bar']
except KeyError:
    pass

# pop equivalent
foo.pop('bar', None)

# suppress equivalent
from contextlib import suppress
with suppress(KeyError):
    foo['bar']

Anyway, don't want to hold up this PR -- more a suggestion for if/when the canvas module gets a refactoring.

[markqvist]

It is very rare, and therefore hard to track down. I have my urwid-based application running on 4 different systems 24/7, and between the four of them, this probably happens once every 6 weeks or so.

I'm suspicious there's a bug lurking deeper...

I think this could very well be the case, but due to my unfamilarity with the finer points of the Urwid codebase, I have not yet been able to pinpoint that exactly. My suspicion was also a more general idea about what you say here.

Also, rewriting this using weakref.WeakKeyDictionary or even just weakref.finalize seems like a good idea, WDYT? It'll obviate most of the try-excepts and make the module simpler to understand.

Sounds reasonable, but again, my understanding of Urwid as a whole is superficial, so I probably can't say much in that regard. I think that this PR should reasonably alleviate the problem until a potential better fix is implemented.