-
Notifications
You must be signed in to change notification settings - Fork 547
CXX-890 Don't use objects requiring dynamic init/fini for instance::c… #513
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| // Copyright 2016 MongoDB Inc. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| #include <cstdlib> | ||
| #include <memory> | ||
|
|
||
| #include <bsoncxx/stdx/make_unique.hpp> | ||
| #include <bsoncxx/stdx/optional.hpp> | ||
| #include <bsoncxx/stdx/string_view.hpp> | ||
|
|
||
| #include <mongocxx/instance.hpp> | ||
| #include <mongocxx/logger.hpp> | ||
| #include <mongocxx/pool.hpp> | ||
| #include <mongocxx/stdx.hpp> | ||
| #include <mongocxx/uri.hpp> | ||
|
|
||
| namespace { | ||
|
|
||
| // This example demonstrates how one might keep a heap allocated mongocxx::instance and | ||
| // mongocxx::pool object associated in a way that allows dynamic configuration. Most of the examples | ||
| // simply create a stack allocated mongocxx::instance object, which is fine for small examples, but | ||
| // real programs will typically require shared access to a commonly configured instance and | ||
| // connection pool across different translation units and components. By providing a singleton which | ||
| // owns the instance and pool objects, we can defer configuration until we are ready to use MongoDB, | ||
| // and share the instance and pool objects between multiple functions. | ||
|
|
||
| class mongo_access { | ||
| public: | ||
|
|
||
| static mongo_access& instance() { | ||
| static mongo_access instance; | ||
| return instance; | ||
| } | ||
|
|
||
| void configure(std::unique_ptr<mongocxx::instance> instance, | ||
| std::unique_ptr<mongocxx::pool> pool) { | ||
| _instance = std::move(instance); | ||
| _pool = std::move(pool); | ||
| } | ||
|
|
||
| using connection = mongocxx::pool::entry; | ||
|
|
||
| connection get_connection() { | ||
| return _pool->acquire(); | ||
| } | ||
|
|
||
| mongocxx::stdx::optional<connection> try_get_connection() { | ||
| return _pool->try_acquire(); | ||
| } | ||
|
|
||
| private: | ||
| mongo_access() = default; | ||
|
|
||
| std::unique_ptr<mongocxx::instance> _instance = nullptr; | ||
| std::unique_ptr<mongocxx::pool> _pool = nullptr; | ||
| }; | ||
|
|
||
| } // namespace | ||
|
|
||
| // The 'configure' and 'do_work' functions use the same mongocxx::instance and mongocxx::pool | ||
| // objects by way of the mongo_access singleton. | ||
|
|
||
| void configure(mongocxx::uri uri) { | ||
| class noop_logger : public mongocxx::logger { | ||
| public: | ||
| virtual void operator()(mongocxx::log_level, mongocxx::stdx::string_view, | ||
| mongocxx::stdx::string_view message) noexcept {} | ||
| }; | ||
|
|
||
| mongo_access::instance().configure( | ||
| mongocxx::stdx::make_unique<mongocxx::instance>(mongocxx::stdx::make_unique<noop_logger>()), | ||
| mongocxx::stdx::make_unique<mongocxx::pool>(std::move(uri)) | ||
| ); | ||
|
|
||
| } | ||
|
|
||
| bool do_work() { | ||
| auto connection = mongo_access::instance().get_connection(); | ||
| if (!connection) | ||
| return false; | ||
| return true; | ||
| } | ||
|
|
||
| int main(int argc, char* argv[]) { | ||
| auto uri = mongocxx::uri{(argc >= 2) ? argv[1] : mongocxx::uri::k_default_uri}; | ||
| configure(std::move(uri)); | ||
| return do_work() ? EXIT_SUCCESS : EXIT_FAILURE; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,9 @@ | |
|
|
||
| #include <mongocxx/instance.hpp> | ||
|
|
||
| #include <atomic> | ||
| #include <mutex> | ||
| #include <type_traits> | ||
| #include <utility> | ||
|
|
||
| #include <bsoncxx/stdx/make_unique.hpp> | ||
|
|
@@ -60,9 +62,14 @@ void user_log_handler(::mongoc_log_level_t mongoc_log_level, const char *log_dom | |
| stdx::string_view{log_domain}, stdx::string_view{message}); | ||
| } | ||
|
|
||
| std::recursive_mutex instance_mutex; | ||
| instance *current_instance = nullptr; | ||
| std::unique_ptr<instance> global_instance; | ||
| // A region of memory that acts as a sentintel value indicating that an instance object is being | ||
| // destroyed. We only care about the address of this object, never its contents. | ||
| typename std::aligned_storage<sizeof(instance), alignof(instance)>::type sentinel; | ||
|
|
||
| std::atomic<instance*> current_instance{nullptr}; | ||
| static_assert(std::is_standard_layout<decltype(current_instance)>::value, "Must be standard layout"); | ||
| static_assert(std::is_trivially_constructible<decltype(current_instance)>::value, "Must be trivially constructible"); | ||
| static_assert(std::is_trivially_destructible<decltype(current_instance)>::value, "Must be trivially destructible"); | ||
|
|
||
| } // namespace | ||
|
|
||
|
|
@@ -94,31 +101,32 @@ instance::instance() : instance(nullptr) { | |
| } | ||
|
|
||
| instance::instance(std::unique_ptr<logger> logger) { | ||
| std::lock_guard<std::recursive_mutex> lock(instance_mutex); | ||
| if (current_instance) { | ||
| throw logic_error{error_code::k_instance_already_exists}; | ||
|
|
||
| while (true) { | ||
| instance* expected = nullptr; | ||
| if (current_instance.compare_exchange_strong(expected, this)) | ||
| break; | ||
| if (expected != reinterpret_cast<instance*>(&sentinel)) | ||
| throw logic_error{error_code::k_instance_already_exists}; | ||
| } | ||
|
|
||
| _impl = stdx::make_unique<impl>(std::move(logger)); | ||
| current_instance = this; | ||
| } | ||
|
|
||
| instance::instance(instance &&) noexcept = default; | ||
| instance &instance::operator=(instance &&) noexcept = default; | ||
|
|
||
| instance::~instance() { | ||
| std::lock_guard<std::recursive_mutex> lock(instance_mutex); | ||
| if (current_instance != this) std::abort(); | ||
| current_instance.store(reinterpret_cast<instance*>(&sentinel)); | ||
| _impl.reset(); | ||
| current_instance = nullptr; | ||
| current_instance.store(nullptr); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be moved up a line, above _impl.reset()
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And, actually... it can't be. There is no proper sequencing of the reset on the _impl and nulling out the current_instance pointer. I think this may be the end for
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if you can handle this with a little state machine. I.e. Create a synthetic instance* that implies the shutting down state and make instance::current spin on it if current_instance.load() returns that specific value. That way you can:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @hanumantmk I tried out your idea, PTAL. I'm not sure if it is better or not at this point. |
||
| } | ||
|
|
||
| instance &instance::current() { | ||
| std::lock_guard<std::recursive_mutex> lock(instance_mutex); | ||
| if (!current_instance) { | ||
| global_instance.reset(new instance); | ||
| current_instance = global_instance.get(); | ||
| if (!current_instance.load()) { | ||
| static instance the_instance; | ||
| } | ||
| return *current_instance; | ||
| return *current_instance.load(); | ||
| } | ||
|
|
||
| MONGOCXX_INLINE_NAMESPACE_END | ||
|
|
||
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.
This incurs the lock cost on every access. How about the cheaper double-checked locking pattern instead?
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.
Double checked locking is unsafe in C++11 without fences: https://en.wikipedia.org/wiki/Double-checked_locking
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.
We could easily enough re-write it to use the Meyer's singleton via magic-statics, if you think that is clearer
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.
Some more details on that: http://preshing.com/20130930/double-checked-locking-is-fixed-in-cpp11/
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.
I updated it to use a magic-statics based singleton.