[BugFix] Check the datacache usability before calling its function in case that the uninitialized members are called in some cases. #51158
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![starrocks-cr[bot]](https://avatars.githubusercontent.com/in/404334?s=60&v=4){kind=small}
be/src/block_cache/block_cache.cpp
Outdated
| if (_kv_cache) { | ||
| return _kv_cache->engine_type(); | ||
| } | ||
| return DataCacheEngineType::STARCACHE; | ||
| } | ||
|
|
||
| void BlockCache::_refresh_quota() { |
There was a problem hiding this comment.
The most risky bug in this code is:
Accessing _kv_cache without checking for its initialization state.
You can modify the code like this:
void BlockCache::record_read_remote(size_t size, int64_t latency_us) {
if (_kv_cache) {
_kv_cache->record_read_remote(size, latency_us);
}
}
const DataCacheMetrics BlockCache::cache_metrics(int level) const {
if (_kv_cache) {
return _kv_cache->cache_metrics(level);
}
DataCacheMetrics dummy_metrics;
return dummy_metrics;
}
void BlockCache::record_read_cache(size_t size, int64_t latency_us) {
if (_kv_cache) {
_kv_cache->record_read_cache(size, latency_us);
}
}
DataCacheEngineType BlockCache::engine_type() {
if (_kv_cache) {
return _kv_cache->engine_type();
}
return DataCacheEngineType::STARCACHE;
}Make sure additional functions also properly check _kv_cache before use to avoid potential issues.
trueeyu
reviewed
Sep 19, 2024
be/src/block_cache/block_cache.cpp
Outdated
| @@ -66,6 +66,9 @@ Status BlockCache::init(const CacheOptions& options) { | |||
|
|
|||
| Status BlockCache::write_buffer(const CacheKey& cache_key, off_t offset, const IOBuffer& buffer, | |||
| WriteCacheOptions* options) { | |||
| if (!_kv_cache) { | |||
There was a problem hiding this comment.
Too many If
Changed it to check before calling datacache functions.
… case that the uninitialized members are called in some cases. Signed-off-by: GavinMar <[email protected]>
GavinMar
force-pushed
the
check_datacache_usability
branch
from
23712b9
to
5d1eba2
Compare
GavinMar
changed the title
trueeyu
approved these changes
Sep 19, 2024
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[BE Incremental Coverage Report]❌ fail : 0 / 1 (00.00%) file detail
|
stephen-shelby
approved these changes
Sep 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why I'm doing:
Now we call datacache
update_mem_quotafunctions to release the memory resource before coredump. However, in some cases, especially for unittests, the datacache instance may not be initialized, which cause an unintialized datacache instance is used and result in unexpected error.What I'm doing:
Check the datacache usability before calling its function.
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: