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
GEODE-8457: Fix crash when IO_error and single-hop=false #642
Conversation
cppcache/src/ThinClientPoolDM.cpp
Outdated
LOGINFO("Removing bucketServerLocation %s due to GF_IOERR", | ||
sl->toString().c_str()); | ||
m_clientMetadataService->removeBucketServerLocation(sl); | ||
if (m_clientMetadataService != nullptr) { |
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.
When I see an if
statement, I usually think there's a possible test case. One for when the statement is true
, and one for false
. Is it possible to write a test for this and the below if
statements?
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.
The m_clientMetadataService member variable is null when single-hop is not enabled.
I have duplicated the test cases I created to verify the fix for GEODE-8231 which had single-hop enabled, to also test the case when single-hop is disabled.
cppcache/src/ThinClientPoolDM.cpp
Outdated
LOGINFO("Removing bucketServerLocation %s due to GF_IOERR", | ||
sl->toString().c_str()); | ||
m_clientMetadataService->removeBucketServerLocation(sl); | ||
if (m_clientMetadataService != nullptr) { |
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.
The one style thing I would point out is that std::unique_ptr
explicitly converts to true
if set or false
if is nullptr
specifically for the usage in if statements like this.
if (m_clientMetadataService) {
m_clientMetadataService->doSomething();
}
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.
ok
- Added 2 test cases and style changes after review
- Added 2 test cases and style changes after review
No description provided.