[PR] Fix cache test reenable loop in VC_EVENT_READ_READY handler [trafficserver]

["bryancall (via GitHub)" <gi...@apache.org>]

bryancall opened a new pull request, #12712:
URL: https://github.com/apache/trafficserver/pull/12712

   ## Problem
   
   The `test_cache_Populated_Cache` unit test was failing with "Subprocess aborted" showing thousands of repeated reenable calls on the same CacheVC object. The Jenkins CI logs showed an infinite loop of reenable messages:
   
   https://ci.trafficserver.apache.org/job/master/job/os_build/47228/console
   
   ## Root Cause
   
   In `src/iocore/cache/unit_tests/main.cc`, the `CacheReadTest::read_event` handler was calling `process_event()` **inside the while loop** that consumes data blocks:
   
   ```cpp
   case VC_EVENT_READ_READY: {
     while (this->_reader->block_read_avail()) {
       // ... consume block ...
       this->process_event(event);  // ❌ Called for EVERY block
     }
     break;
   }
   ```
   
   This meant `reenable()` was being called for every block of data, which is incorrect. When reading from a populated cache with many blocks, this resulted in excessive reenables and triggered the test abort.
   
   ## The Fix
   
   Move the `process_event()` call **outside the while loop** so `reenable()` is only called **once per event** after all available data has been consumed:
   
   ```cpp
   case VC_EVENT_READ_READY: {
     while (this->_reader->block_read_avail()) {
       // ... consume block ...
     }
     this->process_event(event);  // ✅ Called once after consuming all data
     break;
   }
   ```
   
   This matches the pattern used in `CacheTestSM` (the production regression test code) and follows standard async I/O patterns: consume all available data, then signal readiness for more.
   
   ## Testing
   
   - ✅ `test_cache_Cache` - Passed (fresh cache)
   - ✅ `test_cache_Populated_Cache` - **Passed** (previously failing)
   
   The fix reduces reenable calls from thousands to just a handful per test run.


-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix cache test reenable loop in VC_EVENT_READ_READY handler [trafficserver]

["bryancall (via GitHub)" <gi...@apache.org>]

bryancall merged PR #12712:
URL: https://github.com/apache/trafficserver/pull/12712


-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix cache test reenable loop in VC_EVENT_READ_READY handler [trafficserver]

["bryancall (via GitHub)" <gi...@apache.org>]

bryancall commented on PR #12712:
URL: https://github.com/apache/trafficserver/pull/12712#issuecomment-3604132007

   [approve ci rocky]


-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix cache test reenable loop in VC_EVENT_READ_READY handler [trafficserver]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #12712:
URL: https://github.com/apache/trafficserver/pull/12712#discussion_r2582463017


##########
src/iocore/cache/unit_tests/main.cc:
##########
@@ -324,14 +324,14 @@ CacheReadTest::read_event(int event, void *e)
       if (memcmp(str.data(), this->_cursor, str.size()) == 0) {
         this->_reader->consume(str.size());
         this->_cursor += str.size();
-        this->process_event(event);
       } else {
         CHECK(false);
         this->close();
         TEST_DONE();
         break;

Review Comment:
   The `process_event(event)` call should not be executed when the error path (lines 328-331) is taken. After `close()` is called on line 329, `vc` and `vio` are set to `nullptr`, which will cause `REQUIRE` failures in `handle_cache_event()` when processing `VC_EVENT_READ_READY`. Consider restructuring to skip `process_event()` after error handling:
   
   ```cpp
   bool has_error = false;
   while (this->_reader->block_read_avail()) {
     auto str = this->_reader->block_read_view();
     if (memcmp(str.data(), this->_cursor, str.size()) == 0) {
       this->_reader->consume(str.size());
       this->_cursor += str.size();
     } else {
       CHECK(false);
       this->close();
       TEST_DONE();
       has_error = true;
       break;
     }
   }
   if (!has_error) {
     this->process_event(event);
   }
   ```
   
   Alternatively, use `return 0;` instead of `break;` on line 331 to exit the handler early.
   ```suggestion
           return 0;
   ```



-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org