Skip to content
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

CXX-890 Don't use objects requiring dynamic init/fini for instance::c… #513

Merged
merged 1 commit into from Sep 9, 2016

Conversation

acmorrow
Copy link
Contributor

…urrent

@acmorrow
Copy link
Contributor Author

@xdg @jrassi @hanumantmk @Machyne

This is sort of a work-in-progress. Please read CXX-890 for background on why the existing approach for implementing instance::current was problematic in a static-linking context.

I'm not 100% sure that we want to go this way. If we go the other way, we would just remove instance::current entirely.

To close the ticket we will still need to write up an example demo'ing how to manage a long lived instance object, per @bazineta's request.

_impl.reset();
current_instance = nullptr;
current_instance.store(nullptr);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be moved up a line, above _impl.reset()

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 instance::current

Copy link
Contributor

Choose a reason for hiding this comment

The 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:

  1. Set the current_instance over to the shutting down state (which forces a spin)
  2. Run the libmongoc cleanup code
  3. Set the current_instance to null
  4. Allow the caller of instance::current in and allow it to create a new instance (which re-initializes)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

@xdg
Copy link
Contributor

xdg commented Aug 8, 2016

I don't think the complexity is worth it. If users want global state, let's let them design it how they want rather than force them to use/work-around a singleton design.

@acmorrow
Copy link
Contributor Author

acmorrow commented Sep 8, 2016

@xdg - Having instance::current does two very helpful things: it makes it much easier to write unit tests without requiring a fixture to ensure initialization, and it allows a user who has already built a facility to properly initialize the driver, via their own mechanism, to retrieve elsewhere in their code the initialized instance of the driver, without that code needing to depend on (or even know about) the initialization mechanism used. If it is possible to preserve it, I think it has value, even if it costs a little bit of complexity.

@acmorrow
Copy link
Contributor Author

acmorrow commented Sep 8, 2016

@jrassi @xdg @hanumantmk - I've updated this with a full examples showing how to manage an instance/pool pair via a singleton, as was requested by @bazineta in CXX-890. I've also verified that with the existing implementation of mongocxx::instance::current on master, I can reproduce the crash when the recursive mutex is accessed during static destruction, by building the new example against a static libmongocxx.a. After the changes in this commit, the example no longer crashes when statically linked.

@acmorrow
Copy link
Contributor Author

acmorrow commented Sep 8, 2016

Please note that this is the last open issue blocking a C++11 driver 3.0.2 release, so prompt feed back would be appreciated.

return *_singleton;
}

public:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are there duplicate public and private labels?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No good reason, really. I'll remove them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@xdg
Copy link
Contributor

xdg commented Sep 8, 2016

I want to think more about instance::current. My inclination is still to leave it to users to implement (possibly based on the example) if they want. If the major use is for testing, then we can handle that via something private, not something public.

@acmorrow
Copy link
Contributor Author

acmorrow commented Sep 8, 2016

We can re-evaluate whether we want to keep it in 3.0.3 or 3.1. For now, this fixes a bug that makes use of mongocxx in static libraries problematic, even if you aren't using instance::current. I think it is better to fix the bug now, and evaluate the feature separately, and later.

@xdg
Copy link
Contributor

xdg commented Sep 9, 2016

I'm fine with this fix going into 3.0.2. I do think we should re-evaluate. I think we may want to separate mongoc init/cleanup from setting/changing the logging function and that might be a cleaner model. I'll try to articulate something in a ticket soonish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants