Kyle Stanley <aeros...@gmail.com> 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 <rep...@bugs.python.org> <https://bugs.python.org/issue32309> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com