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.

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

GEODE-8613: Removing exclusive access to creation/closing of pool connections #676

Merged
merged 1 commit into from Oct 21, 2020
Merged

GEODE-8613: Removing exclusive access to creation/closing of pool connections #676

merged 1 commit into from Oct 21, 2020

Conversation

albertogpz
Copy link
Contributor

This PR removes the exclusive access to the creation/closing of pool connections that uses the mutex that is also used to grant exclusive access to the ConnectionQueue.
That way access to the connection queue is not impaired when there are problems creating connections.

@albertogpz albertogpz changed the title Feature/geode 8613 GEODE-8613 Oct 15, 2020
@albertogpz albertogpz changed the title GEODE-8613 GEODE-8613: Removing exclusive access to creation/closing of pool connections Oct 15, 2020
jake-at-work
jake-at-work requested changes Oct 15, 2020
View reviewed changes
Copy link
Contributor

@jake-at-work jake-at-work left a comment

Choose a reason for hiding this comment

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

I need more time to review this for correctness. I think the locking is required here to both protect concurrent updates to the m_poolSize value in various places as well as synchronize the CPU caches of this member.

@albertogpz
Copy link
Contributor Author

I need more time to review this for correctness. I think the locking is required here to both protect concurrent updates to the m_poolSize value in various places as well as synchronize the CPU caches of this member.

Wouldn't that already be guaranteed given that the type of m_poolSize is std::atomic<int32_t>?

@jake-at-work
Copy link
Contributor

I need more time to review this for correctness. I think the locking is required here to both protect concurrent updates to the m_poolSize value in various places as well as synchronize the CPU caches of this member.

Wouldn't that already be guaranteed given that the type of m_poolSize is std::atomic<int32_t>?

I hadn't looked into the type yet but mostly yes that should be safe. Though if the pool and count are mutated independently there could be some issues with a mismatch. I will poke into this later today.

jake-at-work
jake-at-work approved these changes Oct 16, 2020
View reviewed changes
Copy link
Contributor

@jake-at-work jake-at-work left a comment

Choose a reason for hiding this comment

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

Looks like we mutate both the poolsize and queue under lock still so I am cool. We might spuriously go into the connection creation methods now but that's ok for this simplification of the locking. This whole chunk of code is begging for a refactor. Perhaps something closer to what we did on the Java side.

@mkevo mkevo merged commit 1f2fc24 into apache:develop Oct 21, 2020
albertogpz added a commit to Nordix/geode-native that referenced this pull request Oct 22, 2020
@albertogpz
GEODE-8613: Removing exclusive access to creation/closing of pool con…
3364019 
…nections (apache#676)
jvarenina pushed a commit to Nordix/geode-native that referenced this pull request Oct 28, 2020
@albertogpz @jvarenina
GEODE-8613: Removing exclusive access to creation/closing of pool con…
632ed92 
…nections (apache#676)
jvarenina pushed a commit to Nordix/geode-native that referenced this pull request Oct 28, 2020
@albertogpz @jvarenina
GEODE-8613: Removing exclusive access to creation/closing of pool con…
6fbd7de 
…nections (apache#676)
@gaussianrecurrence gaussianrecurrence mentioned this pull request Nov 19, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jake-at-work jake-at-work jake-at-work approved these changes

@mkevo mkevo mkevo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@albertogpz @jake-at-work @mkevo