-
-
Notifications
You must be signed in to change notification settings - Fork 764
Replace tornado.locks with asyncio for Events/Locks/Conditions/Semaphore #3397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
9a8bbab
Replace tornado.locks with asyncio for Events/Locks/Conditions/Semaphore
mrocklin 574f335
await conditions in variable.py
mrocklin c302b8a
remove datetime timeouts in queues.py
mrocklin ae23788
Cleanup locks in client.py
mrocklin 0cfe85c
Support Python 3.6
mrocklin 6c0f95f
use asyncio Queue
mrocklin 513a4e7
use asyncio.Event in comm/inproc.py
mrocklin 304d553
Use try-finally around condition waits
mrocklin 4059b05
bump timeout time
mrocklin e40ed0e
use loop.time
mrocklin 998df5d
Handle Python 3.6 again
mrocklin 8d132b5
fixup pubsub.py
mrocklin 271d436
cleanup client.py
mrocklin 1817d4c
bump timeout to 0.1
mrocklin e8f59e0
use IOLoop.time for windows support in test
mrocklin 2fc3471
Windows compat
mrocklin c248d7f
cleanup events in tests
mrocklin 4981994
add back in logging
mrocklin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import asyncio | ||
| import atexit | ||
| from collections import defaultdict | ||
| from collections.abc import Iterator | ||
|
|
@@ -8,6 +9,7 @@ | |
| import errno | ||
| from functools import partial | ||
| import html | ||
| from inspect import isawaitable | ||
| import itertools | ||
| import json | ||
| import logging | ||
|
|
@@ -37,12 +39,7 @@ | |
| except ImportError: | ||
| single_key = first | ||
| from tornado import gen | ||
| from tornado.locks import Event, Condition, Semaphore | ||
| from tornado.ioloop import IOLoop | ||
| from tornado.queues import Queue | ||
|
|
||
| import asyncio | ||
| from asyncio import iscoroutine | ||
|
|
||
| from .batched import BatchedSend | ||
| from .utils_comm import ( | ||
|
|
@@ -431,7 +428,7 @@ def _get_event(self): | |
| # (https://github.com/tornadoweb/tornado/issues/2189) | ||
| event = self._event | ||
| if event is None: | ||
| event = self._event = Event() | ||
| event = self._event = asyncio.Event() | ||
| return event | ||
|
|
||
| def cancel(self): | ||
|
|
@@ -470,7 +467,7 @@ def reset(self): | |
| self._event.clear() | ||
|
|
||
| async def wait(self, timeout=None): | ||
| await self._get_event().wait(timeout) | ||
| await asyncio.wait_for(self._get_event().wait(), timeout) | ||
|
|
||
| def __repr__(self): | ||
| return "<%s: %s>" % (self.__class__.__name__, self.status) | ||
|
|
@@ -630,10 +627,6 @@ def __init__( | |
| self._deserializers = deserializers | ||
| self.direct_to_workers = direct_to_workers | ||
|
|
||
| self._gather_semaphore = Semaphore(5) | ||
| self._gather_keys = None | ||
| self._gather_future = None | ||
|
|
||
| # Communication | ||
| self.scheduler_comm = None | ||
|
|
||
|
|
@@ -678,6 +671,10 @@ def __init__( | |
| self._loop_runner = LoopRunner(loop=loop, asynchronous=asynchronous) | ||
| self.loop = self._loop_runner.loop | ||
|
|
||
| self._gather_semaphore = asyncio.Semaphore(5, loop=self.loop.asyncio_loop) | ||
| self._gather_keys = None | ||
| self._gather_future = None | ||
|
|
||
| if heartbeat_interval is None: | ||
| heartbeat_interval = dask.config.get("distributed.client.heartbeat") | ||
| heartbeat_interval = parse_timedelta(heartbeat_interval, default="ms") | ||
|
|
@@ -1182,7 +1179,9 @@ async def _handle_report(self): | |
|
|
||
| try: | ||
| handler = self._stream_handlers[op] | ||
| handler(**msg) | ||
| result = handler(**msg) | ||
| if isawaitable(result): | ||
| await result | ||
| except Exception as e: | ||
| logger.exception(e) | ||
| if breakout: | ||
|
|
@@ -1238,6 +1237,8 @@ def _handle_error(self, exception=None): | |
|
|
||
| async def _close(self, fast=False): | ||
| """ Send close signal and wait until scheduler completes """ | ||
| if self.status == "closed": | ||
|
mrocklin marked this conversation as resolved.
|
||
| return | ||
| self.status = "closing" | ||
|
|
||
| for pc in self._periodic_callbacks.values(): | ||
|
|
@@ -1252,8 +1253,6 @@ async def _close(self, fast=False): | |
| pass | ||
| if self.get == dask.config.get("get", None): | ||
| del dask.config.config["get"] | ||
| if self.status == "closed": | ||
| return | ||
|
|
||
| if ( | ||
| self.scheduler_comm | ||
|
|
@@ -1345,7 +1344,7 @@ def close(self, timeout=no_default): | |
| if self._start_arg is None: | ||
| with ignoring(AttributeError): | ||
| f = self.cluster.close() | ||
| if iscoroutine(f): | ||
| if asyncio.iscoroutine(f): | ||
|
|
||
| async def _(): | ||
| await f | ||
|
|
@@ -1365,6 +1364,7 @@ async def _shutdown(self): | |
| await self.cluster.close() | ||
| else: | ||
| with ignoring(CommClosedError): | ||
| self.status = "closing" | ||
| await self.scheduler.terminate(close_workers=True) | ||
|
|
||
| def shutdown(self): | ||
|
|
@@ -1800,12 +1800,11 @@ async def _gather_remote(self, direct, local_worker): | |
| few. In controls access using a Tornado semaphore, and picks up keys | ||
| from other requests made recently. | ||
| """ | ||
| await self._gather_semaphore.acquire() | ||
| keys = list(self._gather_keys) | ||
| self._gather_keys = None # clear state, these keys are being sent off | ||
| self._gather_future = None | ||
| async with self._gather_semaphore: | ||
| keys = list(self._gather_keys) | ||
| self._gather_keys = None # clear state, these keys are being sent off | ||
| self._gather_future = None | ||
|
|
||
| try: | ||
| if direct or local_worker: # gather directly from workers | ||
| who_has = await retry_operation(self.scheduler.who_has, keys=keys) | ||
| data2, missing_keys, missing_workers = await gather_from_workers( | ||
|
|
@@ -1820,8 +1819,6 @@ async def _gather_remote(self, direct, local_worker): | |
|
|
||
| else: # ask scheduler to gather data for us | ||
| response = await retry_operation(self.scheduler.gather, keys=keys) | ||
| finally: | ||
| self._gather_semaphore.release() | ||
|
|
||
| return response | ||
|
|
||
|
|
@@ -2911,10 +2908,12 @@ async def _restart(self, timeout=no_default): | |
| if timeout == no_default: | ||
| timeout = self._timeout * 2 | ||
| self._send_to_scheduler({"op": "restart", "timeout": timeout}) | ||
| self._restart_event = Event() | ||
| self._restart_event = asyncio.Event() | ||
| try: | ||
| await self._restart_event.wait(self.loop.time() + timeout) | ||
| except gen.TimeoutError: | ||
| await asyncio.wait_for( | ||
| self._restart_event.wait(), self.loop.time() + timeout | ||
| ) | ||
| except TimeoutError: | ||
| logger.error("Restart timed out after %f seconds", timeout) | ||
| pass | ||
| self.generation += 1 | ||
|
|
@@ -4128,7 +4127,7 @@ async def _first_completed(futures): | |
| See Also: | ||
| _as_completed | ||
| """ | ||
| q = Queue() | ||
| q = asyncio.Queue() | ||
| await _as_completed(futures, q) | ||
| result = await q.get() | ||
| return result | ||
|
|
@@ -4199,19 +4198,14 @@ def __init__(self, futures=None, loop=None, with_results=False, raise_errors=Tru | |
| self.queue = pyQueue() | ||
| self.lock = threading.Lock() | ||
| self.loop = loop or default_client().loop | ||
| self.condition = Condition() | ||
| self.condition = asyncio.Condition(loop=self.loop.asyncio_loop) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question about the |
||
| self.thread_condition = threading.Condition() | ||
| self.with_results = with_results | ||
| self.raise_errors = raise_errors | ||
|
|
||
| if futures: | ||
| self.update(futures) | ||
|
|
||
| def _notify(self): | ||
| self.condition.notify() | ||
| with self.thread_condition: | ||
| self.thread_condition.notify() | ||
|
|
||
| async def _track_future(self, future): | ||
| try: | ||
| await _wait(future) | ||
|
|
@@ -4230,7 +4224,10 @@ async def _track_future(self, future): | |
| self.queue.put_nowait((future, result)) | ||
| else: | ||
| self.queue.put_nowait(future) | ||
| self._notify() | ||
| async with self.condition: | ||
| self.condition.notify() | ||
| with self.thread_condition: | ||
| self.thread_condition.notify() | ||
|
|
||
| def update(self, futures): | ||
| """ Add multiple futures to the collection. | ||
|
|
@@ -4297,7 +4294,8 @@ async def __anext__(self): | |
| while self.queue.empty(): | ||
| if not self.futures: | ||
| raise StopAsyncIteration | ||
| await self.condition.wait() | ||
| async with self.condition: | ||
| await self.condition.wait() | ||
|
|
||
| return self._get_and_raise() | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is specifying s
loop=parameter required for usingasyncio.Semaphorehere? I ask because we weren't passing a loop in before and doing is has been depreciated starting in Python 3.8There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This object is created by the user outside of the event loop. The semaphore will grab the event loop of the current thread, which when we're operating in normal synchronous mode, will not be the same event loop as the one that the Client is running on. So here we have to be explicit and specify the event loop that we want to use.
This usually doesn't come up because we create these objects within async functions or functions that are only being called from within the event loop. The client code can get strange in this way due to the two threads that are active (user/jupyter/ipython thread and the event loop thread)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks for the explaination @mrocklin.
Since
Semaphore,Condition, etc. will attach to the current event loop, could temporarily switch the current loop to be the Client'sself.loop.asyncio_loop, create theSemaphore, and then switch the current loop back to what it was before? (Not 100% sure this wouldn't have other consequences, just proposing as a possible option).Alternatively, I'm happy to keep this as is and open a separate issue for working around the
loop=keyword depreciationThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I missed the statement in your original message that this was being deprecated. The clean way to do this is to to move this to some
__await__definition within this class and make sure that we only create the Semaphore in an async function. I can do this, but I wouldn't mind it being in another PR if that's easy.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, that does sound cleaner. A separate PR for that would be appreciated