{"action":"created","issue":{"url":"https://api.github.com/repos/boostorg/serialization/issues/105","repository_url":"https://api.github.com/repos/boostorg/serialization","labels_url":"https://api.github.com/repos/boostorg/serialization/issues/105/labels{/name}","comments_url":"https://api.github.com/repos/boostorg/serialization/issues/105/comments","events_url":"https://api.github.com/repos/boostorg/serialization/issues/105/events","html_url":"https://github.com/boostorg/serialization/pull/105","id":318714514,"node_id":"MDExOlB1bGxSZXF1ZXN0MTg0ODMzMDUz","number":105,"title":"Fix and test singleton implementation","user":{"login":"Flamefire","id":309017,"node_id":"MDQ6VXNlcjMwOTAxNw==","avatar_url":"https://avatars2.githubusercontent.com/u/309017?v=4","gravatar_id":"","url":"https://api.github.com/users/Flamefire","html_url":"https://github.com/Flamefire","followers_url":"https://api.github.com/users/Flamefire/followers","following_url":"https://api.github.com/users/Flamefire/following{/other_user}","gists_url":"https://api.github.com/users/Flamefire/gists{/gist_id}","starred_url":"https://api.github.com/users/Flamefire/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Flamefire/subscriptions","organizations_url":"https://api.github.com/users/Flamefire/orgs","repos_url":"https://api.github.com/users/Flamefire/repos","events_url":"https://api.github.com/users/Flamefire/events{/privacy}","received_events_url":"https://api.github.com/users/Flamefire/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":19,"created_at":"2018-04-29T12:48:21Z","updated_at":"2018-08-22T09:49:47Z","closed_at":null,"author_association":"NONE","pull_request":{"url":"https://api.github.com/repos/boostorg/serialization/pulls/105","html_url":"https://github.com/boostorg/serialization/pull/105","diff_url":"https://github.com/boostorg/serialization/pull/105.diff","patch_url":"https://github.com/boostorg/serialization/pull/105.patch"},"body":"As described in #104 the singleton implementation was broken in https://github.com/boostorg/serialization/commit/b0a794da38b52c8ae0f8155cf3bb30321cf67f00 which led to crashes especially in the context of (multiple) shared libraries.\r\n\r\nThe \"fix\" allocating the instance on the heap basically just never freed the instances and hence leaked memory. This was due to not understanding the root cause of the problem.\r\n\r\nTo summarize:   \r\nThere are 2 use cases for the singleton: On where the type `T` is derived from `singleton<T>` and one where `singleton<T>` is used without deriving `T` from it. The above commit put the `is_destroyed` handling into the ctor/dtor of `singleton<T>`. However if `T` is not derived from it, then `singleton<T>` is never instantiated and hence `is_destroyed` is never set. See also some discussion in #104 and #79.\r\n\r\nThe crash/memory corruption now happens (or happened before the leaky implementation) if the `tkmap` used from `extended_type_info_typeid` is destroyed first. As it used from the ctor of `extended_type_info_typeid` this should normally not happen. However it does happen when used from multiple shared libraries under (at least) GCC 5.4 and 7.1. The reason is currently unknown (see https://stackoverflow.com/questions/50064617/order-of-destruction-for-static-function-members-in-shared-libraries) but we can just assume, the order of destruction is undefined. Hence the need for the `is_destroyed` flag.\r\n\r\nOn this PR:\r\n\r\n- It is built on top of #94 to enable more testing\r\n- It also tests in release mode to ensure that e.g. stripping of symbols by the linker does not cause problems\r\n- Add a test case to ensure, that singletons are properly constructed, destroyed and the `is_destroyed` flag is correctly set\r\n- Add a test case to test the same for shared libraries\r\n- Add a test case based on https://gist.github.com/Flamefire/286e9e0e501731a04f10786450d3e711 to test the issue with multiple shared libraries. Before this PR this caused a double-free which is detected by the debug-glibc. It would be even better to run this in valgrind, but I'm not familiar enough with the Jam-Files and windows doesn't have it.\r\n- Fix the failing testcases by partially reverting https://github.com/boostorg/serialization/commit/b0a794da38b52c8ae0f8155cf3bb30321cf67f00 :\r\n- Reintroduce `detail:singleton_wrapper` with the correct `is_destroyed` setting in the dtor. It uses a method-local variable to enforce initialization on first use (preferred way to avoid static init disaster https://isocpp.org/wiki/faq/ctors#construct-on-first-use-v2) \r\n- Replace heap-allocated variable by static instance (see isocpp FAQ, avoids static init disaster and is thread safe)\r\n- Assert `!is_destroyed` on enter of `get_instance`. Note that the comment was wrong. It did not refer to the instance and is even removed in non-debug builds\r\n- Make the ctor of `singleton` protected and default to avoid accidental misuse (it might never be called if `T` is not derived from `singleton<T>`)\r\n- Excessively comment everything to avoid breaking changes by misunderstandings\r\n\r\nOther notes:\r\n\r\n- The comment on the heap allocated variable was wrong: as `singleton_wrapper` is the most derived type of the instance it will always be destructed before `T` and `singleton<T>` (if `T` is derived from it) There is nothing one can do about it!\r\n- Valgrind tests would be nice\r\n- static instance is superior to static pointer (see isocpp FAQ), it is not about visibility, double-destruction or anything else. It simply makes sure that the instance is actually destructed at program exit\r\n- Again: It is safe to assume the destruction order is undefined. Hence all asserts assuming otherwise have been removed (they had a runtime check afterwards anyway which is the correct way)\r\n- Not deriving from `singleton<T>` but using it directly is potentially dangerous as you can have multiple `T` instances and you have only 1 `singleton<T>` where you might not expect it. Example: `extended_type_info` uses a `singleton<detail::tkmap>` to work around this issue because `extended_type_info_typeid` already uses a `singleton<tkmap>`. It might be wise to enforce deriving from `singleton<T>` and using `T::get_instance()` or at least do that in the library.\r\n\r\nFinally: Please try to break this! I'm pretty sure I covered everything by using TDD but I might have missed something."},"comment":{"url":"https://api.github.com/repos/boostorg/serialization/issues/comments/414977306","html_url":"https://github.com/boostorg/serialization/pull/105#issuecomment-414977306","issue_url":"https://api.github.com/repos/boostorg/serialization/issues/105","id":414977306,"node_id":"MDEyOklzc3VlQ29tbWVudDQxNDk3NzMwNg==","user":{"login":"Flamefire","id":309017,"node_id":"MDQ6VXNlcjMwOTAxNw==","avatar_url":"https://avatars2.githubusercontent.com/u/309017?v=4","gravatar_id":"","url":"https://api.github.com/users/Flamefire","html_url":"https://github.com/Flamefire","followers_url":"https://api.github.com/users/Flamefire/followers","following_url":"https://api.github.com/users/Flamefire/following{/other_user}","gists_url":"https://api.github.com/users/Flamefire/gists{/gist_id}","starred_url":"https://api.github.com/users/Flamefire/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Flamefire/subscriptions","organizations_url":"https://api.github.com/users/Flamefire/orgs","repos_url":"https://api.github.com/users/Flamefire/repos","events_url":"https://api.github.com/users/Flamefire/events{/privacy}","received_events_url":"https://api.github.com/users/Flamefire/received_events","type":"User","site_admin":false},"created_at":"2018-08-22T09:49:47Z","updated_at":"2018-08-22T09:49:47Z","author_association":"NONE","body":"> I checked #110 with Boost 1.63 and 1.67 with VS 2015 and it runs fine with BOTH versions.\r\n\r\n1.63 is expected to work, 1.65+ is not. If 1.67 worked for you, something is wrong on your side: See appveyor. Did you actually run all the tests? Reconfigured? Started from a clean checkout?\r\n\r\n> this seems to be a case of \"explicitly accessed\"\r\n\r\nYes, missed this in your text. So what was your complaint about this?\r\n\r\n> But here we have crashes of some against memory-leaks of the whole Boost-community, and that's a different story. You shouldn't make your code run on the expense of others.\r\n\r\nDito. You have a memory leak on shutdown or lib unload, which does not matter much, I/we have crashes which are more of a showstopper. So at whose expense should it be?   \r\nBTW: The bugfix is here, the reasons are explained.\r\n\r\n> This is still a non-conforming-compiler problem and should be solved there. Boost allows workarounds for broken compilers, but they should be guarded by respective macros.\r\n\r\nNope, its the runtime for ALL (AFAIK) linux runtimes. And the crash is a use-after-free/destroy which can easily be caught by fixing `is_destroyed` which I did here."}}
{"actor":{"display_login":"Flamefire"}}