Skip to content

Client does not shut down properly when cluster connection attempts all fail #285

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Jun 5, 2017
Merged

Client does not shut down properly when cluster connection attempts all fail #285

merged 10 commits into from
Jun 5, 2017

Conversation

@ihsandemir
Copy link
Collaborator

@ihsandemir ihsandemir commented May 12, 2017

Fix for shutting down the client properly if the cluster connection attempts all fail.

@ihsandemir ihsandemir added this to the 3.8.2 milestone May 12, 2017
@ihsandemir ihsandemir self-assigned this May 12, 2017
@ihsandemir ihsandemir requested a review from sancar May 12, 2017 08:53
@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented May 12, 2017

Test FAILed.

@ihsandemir
Copy link
Collaborator Author

ihsandemir commented May 12, 2017

verify

@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented May 12, 2017

Test FAILed.

1 similar comment
@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented May 12, 2017

Test FAILed.

@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented May 16, 2017

Test FAILed.

1 similar comment
@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented May 18, 2017

Test FAILed.

@ihsandemir
Copy link
Collaborator Author

ihsandemir commented May 18, 2017

verify

@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented May 18, 2017

Test FAILed.

@ihsandemir
Copy link
Collaborator Author

ihsandemir commented May 22, 2017

verify

@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented May 22, 2017

Test FAILed.

@ihsandemir
Copy link
Collaborator Author

ihsandemir commented May 22, 2017

verify

@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented May 22, 2017

Test FAILed.

@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented May 31, 2017

Test FAILed.

1 similar comment
@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented Jun 1, 2017

Test FAILed.

@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented Jun 1, 2017

Test FAILed.

ihsandemir added 5 commits June 1, 2017 18:36
@ihsandemir
Fix for shutting down the client properly if the cluster connection a…
f4ce583 
…ttempts all fail. The Thread join now will detect if it is being called from running thread and avoid deadlock.

Removed the unneded test testTcpSocketConnectionTimeout_withIntMax.

Added some additional tests for testing thread functionality.
@ihsandemir
Rebased with master.
b4527c5
@ihsandemir
Corrected the shutdown order of services, changed to be the same as J…
dbd4c6c 
…ava client.

Corrected what is being tested at IssueTest.issue221.
@ihsandemir
Prevent client destruction before shutdown is completed. Failing to d…
468d7e0 
…o this causes invalid memory access since the client is destroyed and all its services are destroyed.
@ihsandemir
Fixes when IOSelector sets isAlive to true and prevents wakeup if the…
01d1253 
… wakeup socket is not initialized yet.
@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented Jun 1, 2017

Test FAILed.

@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented Jun 1, 2017

Test FAILed.

@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented Jun 2, 2017

Test PASSed.

@sancar
Copy link
Contributor

sancar commented Jun 2, 2017

I did not get why shutdown needs a countdown latch to wait.
Can you describe failing scenario ?
First thing comes to my mind that a thread called shutdown and running shutdown method. In the meantime, destructor called shutdown. Checked that it is not active and returned immediately causing to delete everything while another thread accessing the fields of it.
But in this scenario, it means that HazelcastInstance shared among threads without a sharedPtr.

I am trying to understand if this is a problem we need to solve, or is it just a illegal usage.

@ihsandemir
Copy link
Collaborator Author

ihsandemir commented Jun 2, 2017

@sancar Yes, that is exactly the case. I saw it test https://github.com/hazelcast/hazelcast-cpp-client/blob/master/hazelcast/test/src/cluster/ClientConnectionTest.cpp#L48

So, the test finishes and hence the client destructor is executed before the shutdown is actually completed and hence during the shutdown illegal memory access occurs. I don't think it is an illegal use.

Yes, java uses shared references which eliminates the problem. It may also be solved by keeping an internally shared pointer and just returning a wrapper object HazelcastClient to the user, and also keeping a reference inside the clientcontext object which is being used between the threads.

In any case, we need to guarantee that the shutdown is finished and all threads are closed before exiting the destructor.

@sancar
Copy link
Contributor

sancar commented Jun 2, 2017 •
edited
Loading

I still don't get it. How is the HazelcastClient shared between two threads in this test? If it is a raw pointer and one is calling destructor. This usage is illegal.
If it was shared via a shared_ptr<HazelcastClient> then since the one calling shutdown is not decremented the shared_ptr count, no one can call destructor before it finishes.

@ihsandemir
Copy link
Collaborator Author

ihsandemir commented Jun 2, 2017

@sancar Scenario: ClusterListenerThread at this line: https://github.com/ihsandemir/hazelcast-cpp-client/blob/shutdownFix/hazelcast/src/hazelcast/client/connection/ClusterListenerThread.cpp#L88
and HazelcastClient passes this line: https://github.com/hazelcast/hazelcast-cpp-client/blob/master/hazelcast/src/hazelcast/client/HazelcastClient.cpp#L56

But I think I can also fix it by changing the lines 87 and 88 https://github.com/ihsandemir/hazelcast-cpp-client/blob/shutdownFix/hazelcast/src/hazelcast/client/connection/ClusterListenerThread.cpp#L87 .

@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented Jun 2, 2017

Test FAILed.

@ihsandemir
Added connection closing when ConnectionManager is shutting down. Par…
99ec2f5 
…titionService was hung indefinitely on shutdown, could not regenerate the problem locally but this may be possible reason. Failing to shutdown the connection means any waiting invocation may not be set and if partition service is waiting on future.get, it may block indefinitely.
@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented Jun 3, 2017

Test PASSed.

@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented Jun 3, 2017

Test PASSed.

@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented Jun 5, 2017

Test PASSed.

@ihsandemir ihsandemir merged commit 217484b into hazelcast:master Jun 5, 2017
@ihsandemir ihsandemir deleted the shutdownFix branch June 5, 2017 09:20
@ihsandemir ihsandemir mentioned this pull request Jul 12, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@sancar sancar sancar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ihsandemir ihsandemir

Labels

Type: Defect

Projects

None yet

Milestone

3.8.2

Development

Successfully merging this pull request may close these issues.

3 participants

@ihsandemir @devOpsHazelcast @sancar