Could thin pool graceful cleanup thread be instantiated **not** on module import?

[stephen-hansen]

Hi,

I'm looking at the following lines in python-oracledb:

python-oracledb/src/oracledb/impl/thin/pool.pyx

Lines 987 to 995 in d7003f2

	# keep track of which pools need to be closed and ensure that they are closed
	# gracefully when the main thread finishes its work
	pools_to_close = set()
	def close_pools_gracefully():
	cdef ThinPoolImpl pool_impl
	threading.main_thread().join() # wait for main thread to finish
	for pool_impl in list(pools_to_close):
	pool_impl._shutdown()
	threading.Thread(target=close_pools_gracefully).start()

When pool.pyx gets imported (via thin_impl.pyx, and then in general __init__.py, I think), those lines start a background thread that blocks until the main thread is finished, and then cleans up everything in pools_to_close. Following the import chain above I believe this thread is created and starts running on import of python-oracledb itself, even if pools_to_close is never populated.

This is not ideal to me, I don't expect module imports to necessarily spawn threads, or cause other side effects for that matter, at import time. Just as an example, I was working on a process earlier where I want to block SIGUSR1 in every thread in my process. Even though I was setting my main thread to block SIGUSR1, I was doing this in my main() method, and the thread spawned by python-oracledb was created at import prior to my main method even running, so it was not blocking the signals that I had intended it to. For now I am getting around this by setting blocked signals prior to import.

I'm not sure if a separate thread is the best way to clean up the pools, but at least one immediate idea I had here to improve the code here was to only create the cleanup thread the first time either something is added to the pools_to_close, or you do it at either __init__ or __new__ time via e.g. a class singleton, so that the first time a ThinPoolImpl is constructed the cleanup thread is started as a property of the class. The key point here being that I would greatly prefer if the cleanup thread was created closer to usage, when needed. I'm open to other ideas though.

[cjbj]

This issue rings a bell but I can't immediately find the discussion

[anthony-tuininga]

Your request has merit. I will look into this and get back to you if I have any questions!

[anthony-tuininga]

I have pushed a patch that makes this change and have initated a build from which you can download pre-built development wheels once it completes. You can also build from source if you prefer. If you can test your scenario and confirm the patch works as expected, that would be appreciated!

[stephen-hansen]

the patch looks great, thanks so much @anthony-tuininga! I'll try it out tomorrow and get back to you on it.

PS, I did remember just recently that Python's standard lib does have a module to facilitate this sort of module cleanup behavior via the atexit module (https://docs.python.org/3/library/atexit.html). There are some tradeoffs (see the note on the linked page), and in general I'm not 100% certain it would be right here, but could be worthwhile to look into.

[anthony-tuininga]

Yes, I knew about atexit. See the comment in the newly committed code about why that wasn't an option!