Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow RestartableCyclicTaskABC using Windows events to restart #1679

Merged
merged 1 commit into from
Oct 16, 2023
Merged

Allow RestartableCyclicTaskABC using Windows events to restart #1679

merged 1 commit into from
Oct 16, 2023

Conversation

pierreluctg
Copy link
Collaborator

@pierreluctg pierreluctg commented Oct 13, 2023 •
edited
Loading

Allow RestartableCyclicTaskABC using Windows events to restart

Also adding unit test to cover RestartableCyclicTaskABC and avoid this type of bug.

@pierreluctg pierreluctg added this to the Next Release milestone Oct 13, 2023
@pierreluctg pierreluctg marked this pull request as ready for review October 13, 2023 15:38
@pierreluctg
Copy link
Collaborator Author

@zariiii9003 please review and consider for a bug fix release.

@zariiii9003
Copy link
Collaborator

zariiii9003 commented Oct 14, 2023 •
edited
Loading

Semi-related: Could you check why the SimpleCyclicSendTaskTest test is so slow after #1666?
I assume win32event.INFINITE might be a problem, when the timer gets cancelled. Maybe we could just set a multiple of the requested cycle time as the timeout.

Second edit: The win32event.INFINITE is the reason why self.thread.is_alive() returns True when you try to restart.

@pierreluctg
Copy link
Collaborator Author

pierreluctg commented Oct 16, 2023 •
edited
Loading

SimpleCyclicSendTaskTest

You are right, will fix it

@pierreluctg pierreluctg force-pushed the bcm-win-event-resume-bug branch 3 times, most recently from d5bc22a to 3446b9a Compare October 16, 2023 12:44
Allow RestartableCyclicTaskABC using Win event to restart
6a52e80 
Also adding unit test to cover RestartableCyclicTaskABC
@pierreluctg pierreluctg force-pushed the bcm-win-event-resume-bug branch from 3446b9a to 6a52e80 Compare October 16, 2023 12:46
@pierreluctg
Copy link
Collaborator Author

pierreluctg commented Oct 16, 2023 •
edited
Loading

@zariiii9003 please have another look. This should address the issues.

The CancelWaitableTimer function does not change the signaled state of the timer.

So the thread was waiting for ever (win32event.INFINITE) on the cancled event.
This change replace CancelWaitableTimer by SetWaitableTimer with a period of 0.

This is documented in the SetWaitableTimer API doc.

A periodic timer automatically reactivates each time the period elapses, until the timer is canceled using the CancelWaitableTimer function or reset using SetWaitableTimer.

zariiii9003
zariiii9003 approved these changes Oct 16, 2023
View reviewed changes
Copy link
Collaborator

@zariiii9003 zariiii9003 left a comment

Choose a reason for hiding this comment

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

Looks good. The reset with SetWaitableTimer is a nice solution.

@zariiii9003 zariiii9003 merged commit e869fb7 into develop Oct 16, 2023
56 checks passed
@mergify mergify bot deleted the bcm-win-event-resume-bug branch October 16, 2023 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zariiii9003 zariiii9003 zariiii9003 approved these changes

Assignees
No one assigned
Labels
bug critical
Projects
None yet
Milestone
Next Release
Development

Successfully merging this pull request may close these issues.
2 participants
@pierreluctg @zariiii9003