gateway: Test concurrent rinfo for main_thread_only #274
Conversation
|
Looks like something that would benefit from having an intentional test? And maybe some smoke tests with |
zmedico
force-pushed
the
main_thread_only-cache-gateway-rinfo
branch
2 times, most recently
from
513cf7b to
2814ae9
Compare
zmedico
force-pushed
the
main_thread_only-cache-gateway-rinfo
branch
from
2814ae9 to
95a25c3
Compare
|
@bluetech @RonnyPfannschmidt @nicoddemus I can't request reviews in this repo so tagging you instead. This PR looks ready to be checked/merged. |
|
@zmedico Thanks for following up! It seems to me better to merge pytest-dev/pytest-xdist#1072 and not this. It seems intentional that execnet doesn't request rinfo always, allowing the user to choose whether to grab the info from the remote. But this change will "force" it. The issue seems to me to be specific to xdist and pytest plugin architecture, where xdist can't control what another plugin does, otherwise it would simply avoid the issue. WDYT? |
Yes, I agree that is nicer to allow a choice here. I suppose pytest-cov is not in a position to cache the rinfo, so it makes sense for pytest-xdist to handle it for backward compatibility. I've adjusted this PR to just add test_concurrent_rinfo, and perform the rinfo caching for main_thread_only right inside the test, Noted the change here: pytest-dev/pytest-xdist#1071 (comment) |
Cache execnet gateway info during WorkerController setup for backward compatibility, in order to avoid a later main_thread_only deadlock error triggered when pytest-cov calls rinfo after the main thread is already busy. See pytest-dev/execnet#274 for corresponding test case. Fixes: 20e3ac7 ("Use execnet main_thread_only execmodel (pytest-dev#1027)")
Add test coverage for a bug triggered by pytest-cov when it tried to call _rinfo after the gateway was already busy with a remote_exec call: pytest-dev/pytest-xdist#1071
zmedico
force-pushed
the
main_thread_only-cache-gateway-rinfo
branch
from
38c698c to
af78844
Compare
for more information, see https://pre-commit.ci
zmedico
changed the title
|
@zmedico I admit I'm a bit confused what the test is testing. What is the failure it is testing against? |
| # rinfo while the main thread is busy with the pytest-xdist | ||
| # WorkerController's remote_exec call. | ||
| gw._rinfo() | ||
| assert hasattr(gw, "_cache_rinfo") |
There was a problem hiding this comment.
@zmedico I admit I'm a bit confused what the test is testing. What is the failure it is testing against?
If we omit the above gw._rinfo() call then this test case reproduces the underlying issue from pytest-dev/pytest-xdist#1071:
File "execnet/testing/test_gateway.py", line 682, in test_concurrent_rinfo
rinfo = gw._rinfo()
^^^^^^^^^^^
File "execnet/src/execnet/gateway.py", line 87, in _rinfo
self._cache_rinfo = RInfo(ch.receive())
^^^^^^^^^^^^
File "execnet/src/execnet/gateway_base.py", line 934, in receive
raise self._getremoteerror() or EOFError()
execnet.gateway_base.RemoteError: concurrent remote_exec would cause deadlock for main_thread_only execmodel
There was a problem hiding this comment.
Right but that failure is expected right? And we're testing that adding the extra _rinfo call prevents it from happening?
There was a problem hiding this comment.
I'd like to provide some test coverage for the scenario that led to pytest-dev/pytest-xdist#1071, since this sort of coverage could have prevented a broken release of pytest-xdist.
Cache execnet gateway info during WorkerController setup for backward compatibility, in order to avoid a later main_thread_only deadlock error triggered when pytest-cov calls rinfo after the main thread is already busy. See pytest-dev/execnet#274 for corresponding test case. Fixes: 20e3ac7 ("Use execnet main_thread_only execmodel (pytest-dev#1027)")
|
Closed in favor of pytest-dev/pytest-xdist#1072. |
Add test coverage for a bug triggered by pytest-cov when it tried to call _rinfo after the gateway was already busy with a remote_exec call:
pytest-dev/pytest-xdist#1071