Skip to content

Conversation

@Scheremo
Copy link
Contributor

@Scheremo Scheremo commented Oct 10, 2025

Introduce an injectable clock and test-only wait shim to remove real sleeps
from PendingChanges logic in tests, while preserving production behavior.

PendingChangesMap Changes

  • Add an injectable time source:
    • using SteadyClock = std::chrono::steady_clock;
    • using NowFn = std::function<SteadyClock::time_point()>;
    • Store NowFn nowFn and a bool useManualClock in PendingChangesMap.
  • Replace direct calls to std::chrono::steady_clock::now() with nowFn().
  • Add waitForMinMs(uint64_t ms, SteadyClock::time_point start):
    • Prod path: std::this_thread::sleep_for(ms).
    • Test path (useManualClock): busy-wait with std::this_thread::yield() until nowFn() reaches target.
  • New ctor overload for tests:
    • PendingChangesMap(unsigned maxThreads, NowFn now) -> enables manual clock mode.
  • Dtor now calls abort() to ensure all pending work is cleared on teardown.
  • Replace sleep in debounce worker:
    • sleep_for(ms) -> waitForMinMs(ms, scheduleTime).

PendingChangesMap Unit Test Changes

  • Add ManualClock with manual advancement (advanceMs) and now() accessor.
  • Utility advanceTestTime(clock, ms) that advances test time and yields.
  • CallbackCapture::waitFor() now uses a tight fixed 1ms window (no per-call timeout arg).
  • Refactor tests to:
    • Construct PendingChangesMap with manual clock (PendingChangesMap(2, [&]{ return clock.now(); })).
    • Advance time deterministically instead of sleeping.
    • Validate debounce min/obsolete/max-cap behaviors without flakiness.
  • Adjust debounce cap in MaxCapForcesFlushDuringContinuousTyping to 100ms to tighten assertions.
  • Sprinkle end-of-test large time advances to flush any queued workers.

@Scheremo Scheremo force-pushed the pr-fix-unittest-concurrency branch 5 times, most recently from 8c2dbd6 to f9b6de3 Compare October 10, 2025 21:28
@uenoku
Copy link
Member

uenoku commented Oct 11, 2025

Cool, thank you for improving this! We need to make sure not to use raw now()/sleep_for() but that sounds reasonable

@Scheremo
Copy link
Contributor Author

Cool, thank you for improving this! We need to make sure not to use raw now()/sleep_for() but that sounds reasonable

Thanks for spotting the issue and fixing it on short notice!
My current approach is to centralize time control in the unit test, so each thread "runs on unit test time". This should remove any race conditions on time alone. I think the draft is almost complete, so please feel free to let me know if you have any feedback 😄

@Scheremo Scheremo force-pushed the pr-fix-unittest-concurrency branch 6 times, most recently from 6599683 to ebf3012 Compare October 11, 2025 16:55
Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for improving this! Maybe we might want to check abort is called in deconstructor of PendingChangesMap (RAII also works, but I personally prefer requiring explicit abort ).

@Scheremo Scheremo force-pushed the pr-fix-unittest-concurrency branch from ebf3012 to 1ade794 Compare October 13, 2025 06:12
@Scheremo
Copy link
Contributor Author

Scheremo commented Oct 13, 2025

LGTM, thank you for improving this! Maybe we might want to check abort is called in deconstructor of PendingChangesMap (RAII also works, but I personally prefer requiring explicit abort ).

That's a good point; I added a destructor that calls abort. This should help avoid accidental thread orphaning, and if there are no threads to wait on, this should also not cost anything if abort has been called prior to destruction.

@Scheremo Scheremo marked this pull request as ready for review October 13, 2025 09:55
@Scheremo Scheremo force-pushed the pr-fix-unittest-concurrency branch 2 times, most recently from 892431b to 46730e4 Compare October 13, 2025 10:02
Introduce an injectable clock and test-only wait shim to remove real sleeps
from `PendingChanges` logic in tests, while preserving production behavior.

- Add an **injectable time source**:
  - `using SteadyClock = std::chrono::steady_clock;`
  - `using NowFn = std::function<SteadyClock::time_point()>;`
  - Store `NowFn nowFn` and a `bool useManualClock` in `PendingChangesMap`.
- Replace direct calls to `std::chrono::steady_clock::now()` with `nowFn()`.
- Add `waitForMinMs(uint64_t ms, SteadyClock::time_point start)`:
  - **Prod path**: `std::this_thread::sleep_for(ms)`.
  - **Test path** (`useManualClock`): busy-wait with `std::this_thread::yield()` until `nowFn()` reaches target.
- New ctor overload for tests:
  - `PendingChangesMap(unsigned maxThreads, NowFn now)` -> enables manual clock mode.
- Dtor now calls `abort()` to ensure all pending work is cleared on teardown.
- Replace sleep in debounce worker:
  - `sleep_for(ms)` -> `waitForMinMs(ms, scheduleTime)`.

- Add `ManualClock` with manual advancement (`advanceMs`) and `now()` accessor.
- Utility `advanceTestTime(clock, ms)` that advances test time and yields.
- `CallbackCapture::waitFor()` now uses a tight fixed 100ms window (no per-call timeout arg).
- Refactor tests to:
  - Construct `PendingChangesMap` with manual clock (`PendingChangesMap(2, [&]{ return clock.now(); })`).
  - Advance time deterministically instead of sleeping.
  - Validate debounce min/obsolete/max-cap behaviors without flakiness.
- Adjust debounce cap in `MaxCapForcesFlushDuringContinuousTyping` to 1ms to tighten assertions.
- Sprinkle end-of-test large time advances to flush any queued workers.
@Scheremo Scheremo force-pushed the pr-fix-unittest-concurrency branch from 46730e4 to 72bca03 Compare October 13, 2025 10:03
@Scheremo Scheremo merged commit 7eb08b1 into llvm:main Oct 13, 2025
7 checks passed
@Scheremo Scheremo deleted the pr-fix-unittest-concurrency branch October 21, 2025 07:49
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.

2 participants