Destroy the ComputationClient when the program exits #5750
Conversation
|
Let's wait for the GPU tests to finish before merging, the two left running are what segfaulted for me. |
| std::call_once(g_computation_client_once, | ||
| [&]() { g_computation_client = std::move(CreateClient()); }); | ||
| return g_computation_client.load(); | ||
| static auto client = std::unique_ptr<ComputationClient>(CreateClient()); |
There was a problem hiding this comment.
If the static variable is guaranteed to be initialized once, do you still need the function GetComputationClientIfInitialized and the variable std::atomic<bool> g_computation_client_initialized(false);?
There was a problem hiding this comment.
The client is guaranteed to be initialized once on the first time we call into GetComputationClient. There are a couple of calls that want to tell if client has been initialized (ie comparing GetComputationClientIfInitialized to nullptr), but there's not a way I can see to tell if client has been initialized. As soon as you call GetComputationClient, it gets created. That's why I added the extra flag outside of GetComputationClient.
On a second look, I think it would actually be clearer to eliminate ComputationClient* GetComputationClientIfInitialized and replace it with bool ComputationClientIsInitialzed().
* Destroy the ComputationClient when the program exits * Fix extra error when PJRT_DEVICE is not set
* Destroy the ComputationClient when the program exits * Fix extra error when PJRT_DEVICE is not set
* Destroy the ComputationClient when the program exits * Fix extra error when PJRT_DEVICE is not set
* Destroy the ComputationClient when the program exits * Fix extra error when PJRT_DEVICE is not set
* Destroy the ComputationClient when the program exits * Fix extra error when PJRT_DEVICE is not set
* Destroy the ComputationClient when the program exits * Fix extra error when PJRT_DEVICE is not set
ComputationClient::PrepareToExit(). Any non-trivial destruction can be added to the destructor. @jonb377 found that in practice, any attempt to usePrepareToExitresulted in a segfault, likely due to the sequencing of Python'satexitwith other teardown.staticvariable initialization inside ofGetComputationClientis thread safe after C++11: https://codereview.stackexchange.com/a/197494ComputationClientis destroyed by adding a print to the destructor.