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

_read_wait and friends are documented as being exported from top-level curio package, but they aren't #46

Closed
njsmith opened this issue Apr 30, 2016 · 5 comments

Comments

@njsmith
Copy link
Contributor

njsmith commented Apr 30, 2016

This page says:

    await curio._read_wait(fileobj, timeout=None)

But:

In [1]: import curio

In [2]: curio._read_wait
AttributeError: module 'curio' has no attribute '_read_wait'

It seems to be only in curio.kernel._read_wait. I suppose this is a better place for it and the bug is just in the docs, but either way fyi :-)

(Found because I was trying to track down what happens if a timeout expires; it doesn't seem to be mentioned in the reference manual.)

@dabeaz
Copy link
Owner

dabeaz commented Apr 30, 2016

Good catch. The low-level traps are not exported as part of the API normally. I'll fix the docs to make sure that's more clear.

I'm actually kind of curious about the timeout comment though. I've been refactoring virtually all of the timeout handling into a different approach (in the "wip" branch). Were you tracking down a bug in timeout handling or just looking to see how it worked?

@njsmith
Copy link
Contributor Author

njsmith commented May 1, 2016

Were you tracking down a bug in timeout handling or just looking to see how it worked?

In the first instance I was just looking to figure out how the API worked :-).

a different approach

But, this sounds like it could be relevant to my interests...

Notes towards a Curio HTTP server, including some comments on missing timeout-related features: https://github.com/njsmith/h11/blob/master/curiosittp.py

(Context: https://github.com/njsmith/h11/blob/master/README.rst)

@dabeaz
Copy link
Owner

dabeaz commented May 1, 2016

Oh. Interesting. I've actually been bothered a lot by timeout handling in the first version. Partly it's the fact that every single blocking operation can timeout. If I put a timeout option on everything, it makes the API unnecessarily complicated. Also, there's the problem of a cumulative timeout as your describe.

In the current "work in progress" curio, I've taken away the individual timeout features and replaced them with a new call that can work on a single operation like this:

 await timeout_after(5, some_coroutine(...))

Alternatively, you can apply it to a whole block of statements as a context manager

async with timeout_after(5):
         statements
         ...

This latter form is basically the same as the timeout() manager you have in your code (although implemented as part of the kernel itself). I've used the name "timeout_after" to maybe make it more clear that the timeout is a cumulative timeout as opposed to a per-operation timeout.

@njsmith
Copy link
Contributor Author

njsmith commented May 2, 2016

I guess if we just invented the same construct then that's a good sign :-).

I haven't written enough curio code to identify which things are particularly annoying, but it is true that the need for a "cumulative timeout" was one of the first things I barked my shin on -- I wanted to add a timeout argument to get_remote_events, it has two different awaits in it, and it was immediately like "ugh, this is going to be a pain...". But cumulative timeouts, as you call it, are composable. Yay composability. And as I hadn't quite noticed but you point out, this actually would remove the need for me to provide a timeout argument entirely, so even better!

(Terminology nitpicking/brainstorming: I was thinking about the word "deadline" as a possibly useful piece of terminology, emphasizing that the final time is fixed rather than being relative to each operation like a timeout. "Cumulative timeout" makes sense too, though I had to roll it around for a bit in my head before I could really process it. Or what do you think of async with time_budget(5): ...?)

@dabeaz
Copy link
Owner

dabeaz commented May 24, 2016

Original issue on documentation fixed.

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

No branches or pull requests

2 participants