Conversation
|
if anyone needs the old behaviour they can use a import sys
import contextlib
from distributed import Client
import asyncio
@contextlib.asynccontextmanager
async def shield_from_exception(cmgr_factory, /, *args, **kwargs):
err = None
async with cmgr_factory(*args, **kwargs) as v:
try:
yield v
except BaseException as e:
err = e
if err is not None:
raise err
async def amain():
async with shield_from_exception(Client, asynchronous=True) as client:
raise Exception
def main():
return asyncio.run(amain())
if __name__ == "__main__":
sys.exit(main()) |
|
|
||
| assert _close_proxy.mock_calls == [mock.call(fast=True)] | ||
| assert c.status == "closed" | ||
| assert (stop - start) < 2 |
There was a problem hiding this comment.
I couldn't find any tests that tested what _close(fast=True) does:
it just reduces this 2 second timeout to 0 (I think fast=True should be quite a bit more aggressive, eg force close rpc etc)
distributed/distributed/client.py
Line 1584 in c373746
so that's why I used a spy here
There was a problem hiding this comment.
nit: Would you mind incorporating a description of what closing fast means into the docstring for _close? I was wondering the same thing when I started reading this PR.
There was a problem hiding this comment.
introduced here https://github.com/dask/distributed/pull/27/files#diff-ce261e4d4b0185c8defbfcd46ebe8311a3ecb7ab81d653a2c6664c840340ab2cR250-R251 where it was used as a flag to leak tasks - however we now don't leak tasks but instead cancel them and wait for them
There was a problem hiding this comment.
There was a problem hiding this comment.
@hendrikmakait thanks for the review, it helped clarify my thinking on this!
There was a problem hiding this comment.
nit: Would it make sense to rename fast to force?
There was a problem hiding this comment.
Yeah I think "forcefully=True" is probably the best here
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 15 files + 15 15 suites +15 6h 18m 10s ⏱️ + 6h 18m 10s For more details on these failures, see this check. Results for commit fbf31d2. ± Comparison against base commit f07f384. ♻️ This comment has been updated with latest results. |
hendrikmakait
left a comment
There was a problem hiding this comment.
Code looks good to me, could you elaborate on the motivation for this change?
|
|
||
| assert _close_proxy.mock_calls == [mock.call(fast=True)] | ||
| assert c.status == "closed" | ||
| assert (stop - start) < 2 |
There was a problem hiding this comment.
nit: Would you mind incorporating a description of what closing fast means into the docstring for _close? I was wondering the same thing when I started reading this PR.
c373746 to
d6f6561
Compare
inspired by trio.aclose_forcefully
hendrikmakait
left a comment
There was a problem hiding this comment.
LGTM, thanks for adding the explanation. Feel free to ignore my nit, this just crossed my mind after reading your clarification.
|
|
||
| assert _close_proxy.mock_calls == [mock.call(fast=True)] | ||
| assert c.status == "closed" | ||
| assert (stop - start) < 2 |
There was a problem hiding this comment.
nit: Would it make sense to rename fast to force?
Closes #xxxx
pre-commit run --all-files