fix a race in click.testing.StreamMixer's finalization, seen in a multi-threaded context
#2991
Conversation
| @@ -449,7 +449,7 @@ def test_isolation_stderr_errors(): | |||
| with runner.isolation() as (_, err, _): | |||
| click.echo("\udce2", err=True, nl=False) | |||
|
|
|||
| assert err.getvalue() == b"\\udce2" | |||
There was a problem hiding this comment.
With my patch, stream objects yielded from runner.isolation, including err, may be closed as the context manager exits, dereferencing (hence, allowing gc to collect) its internal StreamMixer reference. Comparable tests already do it this way, so I'm hoping that this change is acceptable.
345e42b to
30bb693
Compare
30bb693 to
712f3f4
Compare
click.testing.StreamMixer's finalization, seen in a multi-threaded contextclick.testing.StreamMixer's finalization, seen in a multi-threaded context
…en in a multi-threaded context
712f3f4 to
4679b1c
Compare
|
Looks reasonable to me to be merge as-is: as @neutrinoceros pointed out, this is hard to constantly reproduce in unit tests (which should probably be named stress tests in that case), and I doubt lots of Click users are running the code in an intensive multi-threaded context as @neutrinoceros does. So if this PR is breaking anything, we would have already caught the regression. |
|
I just noticed I accidentally linked #824, but again, my patch doesn't actually fix it, so it should be undone before merge. |
|
@neutrinoceros Can you unlink #824. For some reason I cannot. Once you do that I will hit merge. |
|
Weird. It seems I also can't. |
|
@kdeldycke can you give it a try? |
Blocked for me to: |
xref python/cpython#136248
xref neutrinoceros/inifix#421
The reproducer I used to work on this involves
typerand multi-threading, and isn't consistently failing: I originally only hit the race about once in 10k tries, so this seems difficult to test systematically.I also am not sure this rare bug warrents an entry in
CHANGES.rst, but I'm glad to add it if needed.update: scratch that, this is a similar looking but different error. My patch doesn't fix the reproducer from #824 (comment)
update 2: close #2993