OxBat commented on PR #586: URL: https://github.com/apache/logging-log4cxx/pull/586#issuecomment-3817528460
Agreed, that's a fair point. Since the issue is just the modulo logic getting out of sync with the data layout, the bug is deterministic. We can reproduce it reliably without race conditions. Here is a standalone test case. On master, this asserts or crashes because `close()` tries to drain from empty slots after the resize. With the fix, the data is realigned and everything passes. You can add this to [src/test/cpp/asyncappendertestcase.cpp](https://github.com/apache/logging-log4cxx/blob/master/src/test/cpp/asyncappendertestcase.cpp): ```cpp void testBufferResizeWraparound() { int initialSize = 10; int newSize = 20; auto asyncAppender = std::make_shared<AsyncAppender>(); asyncAppender->setBufferSize(initialSize); auto vectorAppender = std::make_shared<VectorAppender>(); asyncAppender->addAppender(vectorAppender); Pool p; asyncAppender->activateOptions(p); LoggerPtr root = Logger::getRootLogger(); // 1. warmup: push 5 events and let them drain completely. // this moves the internal read head (dispatchedCount) to 5. for (int i = 0; i < 5; ++i) { auto event = std::make_shared<spi::LoggingEvent>( root->getName(), Level::getInfo(), LOG4CXX_STR("Warmup"), spi::LocationInfo::getLocationUnavailable()); asyncAppender->append(event, p); } // simple spin-wait to ensure dispatcher caught up for (int i = 0; i < 100000 && vectorAppender->getVector().size() < 5; ++i) { std::this_thread::yield(); } LOGUNIT_ASSERT_EQUAL((size_t)5, vectorAppender->getVector().size()); // 2. wrap-around: fill the buffer (10 items). // since we started at index 5, the data wraps: [10-14, 5-9] for (int i = 0; i < 10; ++i) { auto event = std::make_shared<spi::LoggingEvent>( root->getName(), Level::getInfo(), LOG4CXX_STR("CrashTest"), spi::LocationInfo::getLocationUnavailable()); asyncAppender->append(event, p); } // 3. resize while wrapped. // without the fix, this leaves data at the old indices, but the read logic // uses the new size modulo, pointing to empty slots. asyncAppender->setBufferSize(newSize); // 4. trigger drain. // master: reads invalid memory/nullptr. // fix: correctly reads realigned data. asyncAppender->close(); LOGUNIT_ASSERT_EQUAL((size_t)15, vectorAppender->getVector().size()); } ``` I'm happy to include this test directly in the PR if that helps the review. -- 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]
