Skip to content

Conversation

Gezi-lzq
Copy link
Contributor

fix #2716

@Gezi-lzq Gezi-lzq requested review from Copilot and superhx and removed request for superhx, woshigaopp and 1sonofqiu July 29, 2025 06:53
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements scheduled cleanup for expired stream readers to fix issue #2716. The solution adds automatic cleanup of expired StreamReader instances that could accumulate and cause memory leaks.

  • Adds scheduled cleanup task that runs every minute to remove expired StreamReaders
  • Implements manual cleanup trigger method for testing expired StreamReader removal
  • Creates comprehensive test coverage for StreamReader lifecycle and cleanup functionality

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
StreamReaders.java Implements scheduled cleanup task and adds testing methods for monitoring active readers
StreamReadersTest.java Adds comprehensive test suite covering reader creation, reuse, manual cleanup, and periodic cleanup
Comments suppressed due to low confidence (1)

s3stream/src/test/java/com/automq/stream/s3/cache/blockcache/StreamReadersTest.java:90

  • The test uses production scheduling intervals which makes tests slow and unreliable. Consider adding a constructor parameter or test-specific configuration to control cleanup intervals for faster, more reliable tests.
        streamReaders = new StreamReaders(Long.MAX_VALUE, objectManager, objectStorage, objectReaderFactory, 2);

superhx
superhx previously approved these changes Jul 29, 2025
Gezi-lzq added 3 commits July 30, 2025 10:30
…aders for faster cleanup testing

Signed-off-by: Gezi-lzq <[email protected]>
…control and remove reflection from tests

- Add Time dependency to StreamReader and StreamReaders for time-related operations
- Update constructors to accept Time, defaulting to Time.SYSTEM
- Replace System.currentTimeMillis() with time.milliseconds() throughout
- Refactor StreamReadersTest to use MockTime for simulating time passage
- Remove reflection-based time manipulation in tests for cleaner and safer testing
@Gezi-lzq Gezi-lzq merged commit 59a92fa into main Jul 30, 2025
6 checks passed
@Gezi-lzq Gezi-lzq deleted the fix/clean-stream-reader branch July 30, 2025 08:55
Gezi-lzq added a commit that referenced this pull request Jul 30, 2025
…ders (#2719)

* fix(streamReader): implement scheduled cleanup for expired stream readers

* fix(streamReader): implement scheduled cleanup for expired stream readers

* fix(streamReader): add missing import statements in StreamReaders and StreamReadersTest

* fix(StreamReadersTest): improve test setup and cleanup logic for stream readers

* test(StreamReadersTest): update expired stream reader cleanup test for manual trigger and faster execution

* style(StreamReadersTest): remove extra blank line in import statements

* test(StreamReadersTest): use reflection to simulate expired stream readers for faster cleanup testing

Signed-off-by: Gezi-lzq <[email protected]>

* refactor(StreamReader, StreamReaders): inject Time for testable time control and remove reflection from tests

- Add Time dependency to StreamReader and StreamReaders for time-related operations
- Update constructors to accept Time, defaulting to Time.SYSTEM
- Replace System.currentTimeMillis() with time.milliseconds() throughout
- Refactor StreamReadersTest to use MockTime for simulating time passage
- Remove reflection-based time manipulation in tests for cleaner and safer testing

---------

Signed-off-by: Gezi-lzq <[email protected]>
Gezi-lzq added a commit that referenced this pull request Jul 30, 2025
…ders (#2719)

* fix(streamReader): implement scheduled cleanup for expired stream readers

* fix(streamReader): implement scheduled cleanup for expired stream readers

* fix(streamReader): add missing import statements in StreamReaders and StreamReadersTest

* fix(StreamReadersTest): improve test setup and cleanup logic for stream readers

* test(StreamReadersTest): update expired stream reader cleanup test for manual trigger and faster execution

* style(StreamReadersTest): remove extra blank line in import statements

* test(StreamReadersTest): use reflection to simulate expired stream readers for faster cleanup testing

Signed-off-by: Gezi-lzq <[email protected]>

* refactor(StreamReader, StreamReaders): inject Time for testable time control and remove reflection from tests

- Add Time dependency to StreamReader and StreamReaders for time-related operations
- Update constructors to accept Time, defaulting to Time.SYSTEM
- Replace System.currentTimeMillis() with time.milliseconds() throughout
- Refactor StreamReadersTest to use MockTime for simulating time passage
- Remove reflection-based time manipulation in tests for cleaner and safer testing

---------

Signed-off-by: Gezi-lzq <[email protected]>
Gezi-lzq added a commit that referenced this pull request Jul 30, 2025
…ders (#2719)

* fix(streamReader): implement scheduled cleanup for expired stream readers

* fix(streamReader): implement scheduled cleanup for expired stream readers

* fix(streamReader): add missing import statements in StreamReaders and StreamReadersTest

* fix(StreamReadersTest): improve test setup and cleanup logic for stream readers

* test(StreamReadersTest): update expired stream reader cleanup test for manual trigger and faster execution

* style(StreamReadersTest): remove extra blank line in import statements

* test(StreamReadersTest): use reflection to simulate expired stream readers for faster cleanup testing

Signed-off-by: Gezi-lzq <[email protected]>

* refactor(StreamReader, StreamReaders): inject Time for testable time control and remove reflection from tests

- Add Time dependency to StreamReader and StreamReaders for time-related operations
- Update constructors to accept Time, defaulting to Time.SYSTEM
- Replace System.currentTimeMillis() with time.milliseconds() throughout
- Refactor StreamReadersTest to use MockTime for simulating time passage
- Remove reflection-based time manipulation in tests for cleaner and safer testing

---------

Signed-off-by: Gezi-lzq <[email protected]>
Gezi-lzq added a commit that referenced this pull request Jul 31, 2025
…ders (#2719) (#2737)

* refactor(StreamReader, StreamReaders): inject Time for testable time control and remove reflection from tests

- Add Time dependency to StreamReader and StreamReaders for time-related operations
- Update constructors to accept Time, defaulting to Time.SYSTEM
- Replace System.currentTimeMillis() with time.milliseconds() throughout
- Refactor StreamReadersTest to use MockTime for simulating time passage
- Remove reflection-based time manipulation in tests for cleaner and safer testing

---------

Signed-off-by: Gezi-lzq <[email protected]>
Gezi-lzq added a commit that referenced this pull request Jul 31, 2025
…ders (#2719) (#2736)

- Add Time dependency to StreamReader and StreamReaders for time-related operations
- Update constructors to accept Time, defaulting to Time.SYSTEM
- Replace System.currentTimeMillis() with time.milliseconds() throughout
- Refactor StreamReadersTest to use MockTime for simulating time passage
- Remove reflection-based time manipulation in tests for cleaner and safer testing

---------

Signed-off-by: Gezi-lzq <[email protected]>
Gezi-lzq added a commit that referenced this pull request Jul 31, 2025
…ders (#2719) (#2735)

- Add Time dependency to StreamReader and StreamReaders for time-related operations
- Update constructors to accept Time, defaulting to Time.SYSTEM
- Replace System.currentTimeMillis() with time.milliseconds() throughout
- Refactor StreamReadersTest to use MockTime for simulating time passage
- Remove reflection-based time manipulation in tests for cleaner and safer testing

---------

Signed-off-by: Gezi-lzq <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StreamReader cleanup logic may lead to resource leaks
2 participants