Skip to content

cql_builder::build fail race#20

Closed
mhfrantz wants to merge 2 commits intoapache:masterfrom
mhfrantz:cql_builder_build_fail_race
Closed

cql_builder::build fail race#20
mhfrantz wants to merge 2 commits intoapache:masterfrom
mhfrantz:cql_builder_build_fail_race

Conversation

@mhfrantz
Copy link
Copy Markdown

This PR contains two commits.

The first commit is a unit test that reproduces a race. One could argue whether this belongs in the unit test suite, since it actually attempts network communication. It uses a port that is expected to contain no server. Perhaps it should be off by default, or moved to integration tests, although it does not require the CCM bridge stuff.

With the unit test, I was able to generate a few different fatal errors. Running the test under valgrind revealed that there were accesses to deleted memory.

The second commit fixes the bug by using Boost enable_shared_from_this. The callbacks registered with the io_service are thus protected from the race because they contain a shared_ptr rather than a raw pointer.

The change goes further by using the same technique with all of the io_service callbacks that I could find. In fact, I think that some of the complexity of _is_disposed and the cql_trashcan could potentially be eliminated. As far as I can tell, those are mechanisms to address races between the io_service thread and sessions that are closed on the main thread.

@jstasiewicz
Copy link
Copy Markdown
Contributor

Thanks that you took the time to fix this. I'll review and merge your changes soon.

@mhfrantz
Copy link
Copy Markdown
Author

@jstasiewicz , it looks like you incorporated these ideas into your "dev" branch over the past week. Do you want me to pull out the unit test into a separate PR, or are you still deciding where it should go?

@jstasiewicz
Copy link
Copy Markdown
Contributor

Your patch was fine; I thought it over and recognized that this is the right time to exterminate all raw pointers (well, most of them ATM). It was a risky challenge, so it began starting from dev branch.

Regarding the test - I'm not a Git virtuoso, so I can either put it by hand into the tests suite or merge some separate PR of current dev (which would include the test). This second option is the way to go if you have some spare time and want to see your name on the list of commiters ;).
Integration tests seem like a good place for it (indeed, we do test some really basic features there - vide exponential_policy_construction_test).

@mpenick
Copy link
Copy Markdown
Contributor

mpenick commented Jun 11, 2014

Hi! Thanks for your pull request. We just released a new beta version of the driver: https://github.com/datastax/cpp-driver/tree/1.0. Work continues on that branch. I will be moving this branch to "deprecated".

@mpenick mpenick closed this Jun 11, 2014
mpenick pushed a commit that referenced this pull request Feb 14, 2020
CPP-890 - Update CCM bridge to work with C* 4.0 alpha releases
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.

3 participants