Skip to content

Conversation

@fjetter
Copy link
Member

@fjetter fjetter commented Mar 29, 2022

Encountered this during #6005

I assume the indentation of the Scheduler.report was done wrong in one of the bigger refactorings going on. The test passed because of two problems with the test

  • the entire body after the c.cancel could run synchronously. Therefore, client c marked the future already as cancelled but Client f didn't receive the message of the scheduler, yet. Therefore the asserts on the FutureState were successful. Since the scheduler didn't actually cancel the key, y could be properly awaited.
  • Since the test didn't wait for neither f nor c to be actually registered, the cancel_key uses it's retry feature that returns immediately and schedules another callback. On busy CI / stress, this callback could be scheduled before the second client is registered to the scheduler, therefore, the second client would never receive the erroneous cancellation report

@github-actions
Copy link
Contributor

github-actions bot commented Mar 29, 2022

Unit Test Results

       15 files  ±0         15 suites  ±0   6h 48m 53s ⏱️ - 14m 55s
  2 793 tests +1    2 713 ✔️  - 1    78 💤 ±0  2 +2 
20 714 runs  +8  19 802 ✔️ +6  910 💤 ±0  2 +2 

For more details on these failures, see this check.

Results for commit 24d8001. ± Comparison against base commit 63cdddd.

♻️ This comment has been updated with latest results.

@fjetter fjetter force-pushed the fix_regression_cancel_key branch from 2e1053c to 24d8001 Compare May 17, 2022 13:45
@fjetter
Copy link
Member Author

fjetter commented May 18, 2022

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