L69: Provide a functionality to cleanup all static-duration objects

[stefan301]

trafficstars

[chwarr]

It looks like after grpc_final_shutdown_library() is called the library cannot be reinitialized as static, init-once objects will have been deleted, like g_default_client_callbacks. Previously, the library could be initialized and destroyed multiple times in the same process. (E.g., init_test.cc). Is this difference intentional? If so, can you add a section that explains the pros/cons of other approaches and why this approach was selected over others?

[chwarr]

I'd like to see a section about testing so that the changes can be validated against the problem they're trying to solve and so that regressions aren't introduced.

[stefan301]

|Yes, after ||grpc_final_shutdown_library() has been called the library cannot be reinitialized again. It is not a replacement for grpc_init()/grpc_shutdown() but an additional cleanup that should only be called just before a memory leak detection starts at the end of the process or when unloading a DLL that uses gRPC. grpc_init() can still be called again after the last |||grpc_shutdown() call. ||

|||That's just what ShutdownProtobufLibrary() does for the protobuf library. |

Am 20.07.2020 um 20:33 schrieb Christopher Warrington:

It looks like after |grpc_final_shutdown_library()| is called the library cannot be reinitialized as static, init-once objects will have been deleted, like |g_default_client_callbacks|. Previously, the library could be initialized and destroyed multiple times in the same process. (E.g., init_test.cc https://github.com/grpc/grpc/blob/03b3a8930e52496c6d324c88f1a360a7f20c765f/test/core/surface/init_test.cc#L70-L83). Is this difference intentional? If so, can you add a section that explains the pros/cons of other approaches and why this approach was selected over others?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/grpc/proposal/pull/199#issuecomment-661260994, or unsubscribe https://github.com/notifications/unsubscribe-auth/AH3YCAFKPY3JP6FODO5TYTTR4SEWRANCNFSM4PBQAC5A.

[chwarr]

Yes, after grpc_final_shutdown_library() has been called the library cannot be reinitialized again. It is not a replacement for grpc_init()/grpc_shutdown() but an additional cleanup that should only be called just before a memory leak detection starts at the end of the process or when unloading a DLL that uses gRPC.

I'd add something like this to the proposal so it's captured there. This behavior is an important part of the proposal.

I'd still also like to see pros/cons of other approaches that were considered and why this one was selected from among them.

E.g., grpc_init could be changed to return a grpc_handle that then gets passed into every function. grpc_shutdown would then destroy all resources associated with that handle. Independent handles are completely different gRPC universes that share nothing. Rejected because this is a breaking change in every API just to handle clean up of some library-wide singletons (like caches).

[stefan301]

Yes, after grpc_final_shutdown_library() has been called the library cannot be reinitialized again. It is not a replacement for grpc_init()/grpc_shutdown() but an additional cleanup that should only be called just before a memory leak detection starts at the end of the process or when unloading a DLL that uses gRPC.

I'd add something like this to the proposal so it's captured there. This behavior is an important part of the proposal.

I'd still also like to see pros/cons of other approaches that were considered and why this one was selected from among them.

E.g., grpc_init could be changed to return a grpc_handle that then gets passed into every function. grpc_shutdown would then destroy all resources associated with that handle. Independent handles are completely different gRPC universes that share nothing. Rejected because this is a breaking change in every API just to handle clean up of some library-wide singletons (like caches).

Thanks for your comments.

Changing grpc_init to return a grpc_handle which gets passed into every function sounds like a lot of work.

I chose this approach because it is the way libprotobuf does it already and it minimises the efford for the libary programmer as well as for the library user. This one is not a breaking change, only one new public function that can be called if needed.

gRPC users that are happy with the current implementation don't have to change anything. Of course they have to pay the price in runtime and memory consumption that it takes to register these cleanup functions, which should be insignificant . Users that want to see their own memory leaks but not the false positives from gRPC can call grpc_final_shutdown_library.

[chwarr]

I'd put basically what you've written in the PR discussion into the proposal itself. That makes the proposal a more self-contained document.

[veblush]

Thank you for the proposal.

I know the benefit of supporting this feature as I used to use the same feature on Windows to detect memory leaks but I'm not sure this fits to gRPC.

1. We're heavily depending on ASAN and MSAN to prevent memory leaks but they don't consider live objects stemming from static variables as leaks since they're still alive technically. We have many tests running with this configuration to detect those problems. Having a slightly different memory testing tool doesn't seem to add great value at the cost of setting it up and running and maintaining it. To have the same level of coverage, all existing tests with ASAN should run with MSVC and debug heap mode.

2. I'm concerned whether it's doable because it depends when the static objects are created. If they're created by explicit functions call like grpc_init(), it'd be fine but if it is created by global or function static variable's constructors, it's fairy complicated to know when they're called. If they're called before the main function, debug heap is okay with not freeing them but it claims they're leaks when it happens to be allocated after the main function. This varies depending on the build configuration, compilers, so on. This usually gives us unpleasant Static Initialization Order Fiasco. So it's not easy to make sure that all static objects are wrapped with the OnShutdownDelete. Moreover, internally we're allowed to write code like the following when it's not trivial to call its destructor safely.

// This is acceptable from totw-110
const std::string& GetKey() {
  static const std::string* const kKey = new std::string("my secret key");
  return *kKey;
}

This could be a blocker for this proposal because it's intentionally not freeing the static variable because the destructor of static variable might assume that there must be a live object referenced by it. This is considered as a memory leak from MSVC debug heap but not from ASAN.

[yang-g]

As mentioned in the issue, I think we should first focus on fixing the original Windows failure under the existing API before considering expanding our API surface. The proposed API is essentially grpc_shutdown_blocking or some variant of it. While it might be helpful to have this kind of API especially in some tests, I do not think it is easy for general users to use it.

[stefan301]

Thank you for your comments,

Where can I find something about totw-110 (at least not here: https://abseil.io/tips/). Isn't that the Construct On First Use Idiom which ShutdownData::get() also uses?

Maybe I missed something. But why isnt't it safe to call the destructor for kkey as one of the last actions before the process exits? Of course you shouldn't call GetKey() after that, but I thinks that's stated clearly for grpc_final_shutdown_library(). I assume that there is not a delete &GetKey(); going on somewhere.

Maybe there could be another solution at least for Windows/MSVC. If all global static allocations would be done with a specific malloc (maybe inside a factory function with placement new) then _malloc_dbg(size,_CRT_BLOCK,..) could be used to tell the debug heap to ignore this "leak". For containers like strings a separate allocator would be needed which changes the type. But I think that wouldn't be feasible.

Am 22.07.2020 um 02:08 schrieb Esun Kim:

Thank you for the proposal.

I know the benefit of supporting this feature as I used to use the same feature on Windows to detect memory leaks but I'm not sure this fits to gRPC.

We're heavily depending on ASAN and MSAN to prevent memory leaks
but they don't consider live objects stemming from static
variables as leaks since they're still alive technically. We have
many tests running with this configuration to detect those
problems. Having a slightly different memory testing tool doesn't
seem to add great value at the cost of setting it up and running
and maintaining it. To have the same level of coverage, all
existing tests with ASAN should run with MSVC and debug heap mode.

I'm concerned whether it's doable because it depends when the
static objects are created. If they're created by explicit
functions call like |grpc_init()|, it'd be fine but if it is
created by global or function static variable's constructors, it's
fairy complicated to know when they're called. If they're called
before the main function, debug heap is okay with not freeing them
but it claims they're leaks when it happens to be allocated after
the main function. This varies depending on the build
configuration, compilers, so on. This usually give us unpleasant
|Static Initialization Order Fiasco|. So it's not easy to make
sure that all static objects are wrapped with the
|OnShutdownDelete|. Internally, we're allowed to write code like
the following when it's not trivial to call its destructor safely.

|// This is acceptable from totw-110 const std::string& GetKey() { static const std::string* const kKey = new std::string("my secret key"); return *kKey; } |

This could be a blocker for this safe guard because it's intentionally not freeing the static variable. This is considered as a memory leak from MSVC debug heap but not from ASAN.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/grpc/proposal/pull/199#issuecomment-662168017, or unsubscribe https://github.com/notifications/unsubscribe-auth/AH3YCADUYX5BAWWL3SFIJULR4YUWPANCNFSM4PBQAC5A.