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.

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

Remove all sleep calls? #2

Open
takluyver opened this issue Oct 10, 2014 · 10 comments
Open

Remove all sleep calls? #2

takluyver opened this issue Oct 10, 2014 · 10 comments

Comments

@takluyver
Copy link
Member

There are still a number of time.sleep() calls, particularly in close() and terminate(). This is bad for async code, because this will hog control when the event loop could be doing other things. I'd like to work out how to remove these. If that isn't possible, it should at least be marked very clearly which methods do this.

@jquast
Copy link
Member

jquast commented Apr 27, 2015

If we used something like Trollius, we could use asyncio even on python2.7 to yield from asyncio.sleep instead.

@takluyver
Copy link
Member Author

The tricky thing is that any code calling a coroutine has to be aware that it's a coroutine, and use the same async framework. So it's not a drop-in replacement for code with time.sleep() calls. But I think I've removed most of the sleeps, and those that are left can wait for a future release.

@elupus
Copy link

elupus commented Aug 24, 2022

We just ran into this on HomeAssistant. Since we can't allow sleeps at all in main event loop (it blocks all other integrations), we check for this and throw exceptions when detected. Why is there even a delayafterclose?

@Red-M
Copy link
Member

Red-M commented Aug 24, 2022

I'm not sure this actually fixable.
We'd have to have some way for the sleep to be controlled by the event loop and it'd have to match your async framework (for older versions of python).

Happy to review PRs and merge them if the PR is up to scratch.

@elupus
Copy link

elupus commented Aug 24, 2022

Ive opened a issue upstream for pipe transport. We solved this in pyserial, but this is core python. python/cpython#96229

@takluyver
Copy link
Member Author

We could now have an async version of this method that calls await anyio.sleep() - AnyIO is a layer that detects which async framework you're running and uses functions from that, so we don't have to pick a specific framework. Admittedly 'any' currently just means asyncio or Trio, but if someone wants to launch another framework, AnyIO is a single point they can focus on getting it supported.

@elupus
Copy link

elupus commented Aug 24, 2022

The problem is that the close function on the pipe is called by cython core loop. So you cant really change it to async. You can possibly postpone this code in some call_later() solution.

@takluyver
Copy link
Member Author

That could be circumvented in principle - e.g. you could have a close_async=True parameter when you instantiate it which would override self.close = self.aclose (or whatever naming). It's obviously not ideal, though.

@elupus
Copy link

elupus commented Dec 26, 2022

Cython issue was closed as wont fix. So must be fixed here. Can this just indicate to caller the need to wait?

@elupus
Copy link

elupus commented Dec 26, 2022

Actually i think pexpect need to make sure here: https://github.com/pexpect/pexpect/blob/2e003effe885a54a320ba0a6eb91751714bc9ad3/pexpect/_async.py#L18

To wrap the close method of ptyprocess, and make sure it is scheduled to be executed in an executor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants