Kyle Stanley <[email protected]> added the comment:
> I don't think changing the default executor is a good approach. What happens,
> if two or more thread pools are running at the same time? In that case they
> will use the same default executor anyway, so creating a new executor each
> time seems like a waste.
I agree that it would be better to have ThreadPool use an internal executor
rather than relying on the event loop's default executor. The main reason I
hadn't was because we hadn't implemented an asynchronous executor shutdown
outside of loop.shutdown_default_executor(), but we could potentially move the
functionality to a private function (in Lib/asyncio/base_events.py) so it's
reusable for ThreadPool. It could be something like this:
async def _shutdown_executor(executor, loop):
future = loop.create_future()
thread = threading.Thread(target=loop._do_shutdown,
args=(executor,future))
thread.start()
try:
await future
finally:
thread.join()
def _do_shutdown(loop, executor, future):
try:
executor.shutdown(wait=True)
loop.call_soon_threadsafe(future.set_result, None)
except Exception as ex:
loop.call_soon_threadsafe(future.set_exception, ex)
Note that the above would be for internal use only, for the existing
loop.shutdown_default_executor() and the new asyncio.ThreadPool. For it to
support both, it would have to accept an explicit loop argument. It also does
not need a robust API, since it's private.
> Shutting down the default executor seems unnecessary and could impact lower
> level code which is using it. The default executor is shutdown at the end of
> asyncio.run anyway.
I agree with your point regarding the shutdown of the default executor one. But
I think we should shutdown the internal executor for the ThreadPool, as a main
point context managers is to start and clean up their own resources. Also, I'm
aware that asyncio.run() shuts down the default executor, I implemented that
fairly recently in GH-15735. ;)
Another substantial concern is in the case of a coroutine that contains
asyncio.ThreadPool being executed without asyncio.run(). There are still use
cases for using the lower level loop.run_until_complete() for more complex
asyncio programs. I don't think we should make asyncio.ThreadPool dependent on
the coroutine being executed with asyncio.run(). Thus, it makes sense that
ThreadPool should create a new instance of ThreadPoolExecutor upon entry of the
context manager and then shut it down upon exit.
> I'm not sure if a new ThreadPoolExecutor really needs to be created for each
> ThreadPool though.
IMO, a context manager should create and then finalize it's own resources,
rather than sharing the same executor across contexts. Sharing the same one
seems to defeat the purpose of using a context manager in the first place, no?
> Run method should be:
async def run(self, func, *args, **kwargs):
call = functools.partial(func, *args, **kwargs)
return await self._loop.run_in_executor(None, call)
Correction: if we're using an internal executor now, this should instead be
self._loop.run_in_executor(self._executor, call). With `None`, it will simply
use the event loop's default executor rather the context manager's
ThreadPoolExecutor.
> `run` should be awaitable method, see #38430
Agreed, good point.
> Paul's version looks better.
I think he had some good points, particularly around using an internal executor
instead the event loop's default executor; but there's some parts that I
disagree with, see above reasons.
> 1. get_running_loop() should be used instead of get_event_loop()
Note: If get_running_loop() is used instead, it has to set self._loop within a
coroutine (since get_running_loop() can only be used within coroutines), that's
why in my version it was within __aenter__. I think this would make the most
sense.
> 2. There is no `await executer.shutdown()` API, the method is synchronous.
That's why in my version I was using the existing event loop API, since we had
already implemented an asynchronous loop.shutdown_default_executor() method
that calls executor.shutdown(). However, if we added a private
_shutdown_executor() and _do_shutdown() as I mentioned above, that wouldn't be
an issue.
Thanks for the feedback on the prototype Paul and Andrew, both of you brought
up some good points. I'll start working on a PR.
----------
_______________________________________
Python tracker <[email protected]>
<https://bugs.python.org/issue32309>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
