-
Notifications
You must be signed in to change notification settings - Fork 566
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
API: Add shutdown method #3882
API: Add shutdown method #3882
Conversation
This change adds a `shutdown` method to the API that can be used to perform shutdown of the thread-pool(s) (or other clean-up activities). Applications that utilize OpenImageIO must call it before exiting their main function to prevent possible deadlocks during application shutdown (see AcademySoftwareFoundation#3851) This change also updates the documentation and all oiio-tools to call the new `shutdown` method. This is the final change for fixing AcademySoftwareFoundation#3851
src/doc/shutdown.rst
Outdated
.. _chap-shutdown: | ||
|
||
Shutdown | ||
######## | ||
|
||
Before exiting an application that utilizes OpenImageIO the `shutdown` function | ||
must be called, which will perform shutdown of any running thread-pools. | ||
Failing to call `shutdown` could lead to a sporadic dead-lock during | ||
application shutdown on certain platforms such as Windows. | ||
|
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.
Rather than make a new chapter, let's put this in the existing imageioapi.rst, perhaps add a "Startup and Shutdown" section?
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.
Code LGTM, but let's move the docs to the existing API chapter as I noted inline.
Should we do anything about Python? Expose OIIO::shutdown() through the python bindings? Or should we ensure that when python module unloads, it will call shutdown? Is there harm in calling shutdown twice()? In calling anything that uses the thread pool after shutdown()? (Does it automatically set it up again?) What are the instructions we should give to a library author who uses OIIO and isn't sure if, at runtime, the app itself uses OIIO, or knows the library uses OIIO, so the app would call shutdown? Or what if the app uses other libraries that use OIIO but each one isn't sure if they are the only one? |
I'll have to admit : not a python guy. If i had to guess I'm guessing when python shuts down it unloads all modules before exiting causing the regular dtor for the threadpool to run long before the process exists so all threads still be alive? so likely not a problem? But I admit wildly guessing here, would have to check with the debugger or sprinkle some printf's to validate this theory. |
Oh, yes, you're probably right. I guess this is only a problem when the process ends naturally but terminates so quickly that the threads are killed before the pool has finished releasing? Let's move forward on this and we can always amend later if there's another edge case we haven't considered. |
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.
LGTM
Description
This change adds a
shutdown
method to the API that can be used to perform shutdown of the thread-pool(s) (or other clean-up activities). Applications that utilize OpenImageIO must call it before exiting their main function to prevent possible deadlocks during application shutdown (see #3851)This change also updates the documentation and all oiio-tools to call the new
shutdown
method.This is the final change for fixing #3851
Tests
No tests added / unsure how to test for it.
Checklist:
have previously submitted a Contributor License Agreement
(individual, and if there is any way my
employers might think my programming belongs to them, then also
corporate). - Minimal change, no CLA required
(adding new test cases if necessary).