airborne12 commented on PR #67720: URL: https://github.com/apache/doris/pull/67720#issuecomment-6499912164
Two notes from #69811, which fixed the same race and which I'm closing in favor of this PR: 1. `be/test/storage/segment/segment_footer_cache_test.cpp` still calls `segment_ptr->_footer_pb.reset();` in `GetSegmentFooterPropagatesIoContext` (added by #65058; it is also at this PR's head, line 175). With `_footer_pb` removed, the BE UT build fails to compile there. The line can simply be dropped: the test erases the footer's cache entry right before it, and without the weak_ptr that alone forces the reload. 2. #69811 added `SegmentFooterCacheTest.ConcurrentGetSegmentFooterWithEviction`: 8 threads call `_get_segment_footer()` and `get_metadata_size()` while half of them keep erasing the footer's cache page. On the old code, ASAN reported a heap-use-after-free within 20 runs: a thread read the footer through `SegmentFooterPB::ByteSizeLong` after another thread had freed it in `std::weak_ptr` assignment, the same frame as the production crashes described above. The test does not touch `_footer_pb`, so it also works with the cache-only design here; feel free to take it: https://github.com/apache/doris/pull/69811/files -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
