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

Fix win sleep bug #2758

Merged
merged 8 commits into from Jun 12, 2023
Merged

Fix win sleep bug #2758

merged 8 commits into from Jun 12, 2023

Conversation

rodrigogiraoserrao
Copy link
Contributor

Fixes #2711

@rodrigogiraoserrao rodrigogiraoserrao marked this pull request as draft June 9, 2023 13:46
willmcgugan
willmcgugan reviewed Jun 9, 2023
View reviewed changes
src/textual/_win_sleep.py Outdated
@@ -37,32 +49,62 @@ def sleep(secs: float) -> None:
sleep_for = max(0, secs - 0.001)
if sleep_for < 0.0005:
# Less than 0.5ms and its not worth doing the sleep
return
return time_sleep_coro(0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this isn't run in a thread context somewhere, it will block all asyncio tasks from running.

If this is ultimately calling time.sleep(0) then it could end up sleeping for several milliseconds. sleep(0) basically means give up your time slice and go run another thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about returning None in this case and then checking for it in _time?
See 0c32c05.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You could just return a coroutine that does nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done e3f9dc1.

@willmcgugan
Copy link
Collaborator

@rodrigogiraoserrao Failing tests there

@rodrigogiraoserrao
Copy link
Contributor Author

@rodrigogiraoserrao Failing tests there

Pushed a possible fix. But what I really need is help with the regression test. That's why this is a draft.
I thought the test test_win_sleep_timer_is_cancellable was what we needed but it isn't.
If you run that test on main, it doesn't fail.
pytest concludes with a green pass and then it hangs for 10s, just like the app in the original issue.

@rodrigogiraoserrao rodrigogiraoserrao marked this pull request as ready for review June 12, 2023 10:31
@rodrigogiraoserrao rodrigogiraoserrao merged commit 14269a4 into main Jun 12, 2023
17 checks passed
@rodrigogiraoserrao rodrigogiraoserrao deleted the win-sleep-bug branch June 12, 2023 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willmcgugan willmcgugan willmcgugan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

On Windows set_interval blocks a full exit of the application until the next interval fires
2 participants
@rodrigogiraoserrao @willmcgugan