Skip to content

Fix error when destructing a static std::unique_ptr<cppflow::model>#132

Closed
ihowell wants to merge 1 commit intoserizba:masterfrom
ihowell:master
Closed

Fix error when destructing a static std::unique_ptr<cppflow::model>#132
ihowell wants to merge 1 commit intoserizba:masterfrom
ihowell:master

Conversation

@ihowell
Copy link

@ihowell ihowell commented Jun 17, 2021

Fixes #131

When creating a static version of a reference to a cppflow::model,
there becomes an error when destructing, due to the
context::get_status() method using a thread_local status. While
this is done normally for performance gains and is fine in most
places. However, when the program is exiting, it first destructs thread_local
objects and then static ones. This leads to local_tf_status
containing a nullptr instead of a pointer to a TF_Status
object. Therefore, TF_DeleteSession(sess, status) fails because it
access *status. The simplest fix is to not re-use this status object
in the destructor of the session and instead make a new,
non-thread_local status instead.

When creating a static version of a reference to a `cppflow::model`,
there becomes an error when destructing, due to the
`context::get_status()` method using a `thread_local` status. While
this is done normally for performance gains and is fine in most
places. However, when the program is exiting, it first destructs `thread_local`
objects and then `static` ones. This leads to `local_tf_status`
containing a `nullptr` instead of a pointer to a `TF_Status`
object. Therefore, `TF_DeleteSession(sess, status)` fails because it
access `*status`. The simplest fix is to not re-use this status object
in the destructor of the session and instead make a new,
non-`thread_local` status instead.
@ljn917 ljn917 mentioned this pull request Sep 27, 2021
@serizba serizba force-pushed the master branch 2 times, most recently from 3326662 to c98a475 Compare February 28, 2022 10:20
@serizba
Copy link
Owner

serizba commented May 25, 2022

Hi @ihowel, thanks for spotting this!

Perhaps is better to allocate on TF_Status per model, as suggested by @ljn917 here.

@ljn917
Copy link
Contributor

ljn917 commented May 25, 2022

@serizba Do you mind me having a PR to fix, i.e. adding a per model TF_Status? The global status will be used for eager ops only. There were so many people trying to build cppflow into a dll and having the deinitialuzation order fiasco.

@serizba
Copy link
Owner

serizba commented May 25, 2022

@ljn917 Yeah sure! This way we'll fix all related issues.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrapping a model in static unique_ptr results in seg fault when destructing

3 participants