dabeaz / curio

Good Curio!
Other
4.02k stars 241 forks source link

Add the ability to customize the task ID from `spawn`. #193

Closed Fuyukai closed 7 years ago

Fuyukai commented 7 years ago

Task had the ability to pass a task ID, but this was never used anywhere.

dabeaz commented 7 years ago

There are a lot of things that could go wrong by specifying an explicit task id. Can you say more about the use-cases of this?

Fuyukai commented 7 years ago

For me, I use the task ID to attach extra information to the task objects due to being slotted.

I just found it weird that it was exposed in the constructor, and not the spawn call.

dabeaz commented 7 years ago

Looking around, it doesn't seem that explicit task_id setting is even relevant. At one time, it was used to set the id of the in-kernel task. However, that code doesn't need that functionality. All things, equal, I'm inclined to chop the feature altogether. Especially since I'm in a "cutting mood" with respect to Curio right now.

dabeaz commented 7 years ago

Forget to add, I can make it so new attributes can be added to Task objects. That's probably not a bad idea--and would be cleaner than trying to manage things in a side-dictionary or something.

Fuyukai commented 7 years ago

That would be better than my bootleg ID modification.

malinoff commented 7 years ago

@dabeaz have you thought about giving an ability to provide a custom Task class? It would cover this use-case, and mine.

I'm working on a sans-io library amqproto which is based on Future objects (instead of on callbacks passed around everywhere). I'd like to implement a curio I/O adapter but to do that I need to make curio Tasks behave like they're futures which isn't really possible right now.

My use-case needs only set_result and __await__ methods implemented, and if I could provide my own Task class it would look like this:

import curio
class AlmostFuture(curio.Task):
    __await__ = curio.Task.join
    def set_result(self, result):
        self.next_value = result

(obviously I need things like curio.spawn to return instances of my AlmostFuture, not curio.Task).

dabeaz commented 7 years ago

How does this modified class intersect with the general usage of Curio? Is the spawn() method simply modified to return this new class? Is it used elsewhere?

njsmith commented 7 years ago

@malinoff I really think you'll be happier if you switch out the subclassing and Futures for a design more like hyper-h2, and then wrap that in io library specific adapters. Futures and callbacks are not idiomatic in curio, or even asyncio really.

malinoff commented 7 years ago

@njsmith AMQP is asynchronous (unlike HTTP). Various events may (or may not) happen at some point in future, when the broker decides to reply back (or not), and there have to be a way to respond to such delayed events. The use of futures is hidden from the user's point of view - I could provide a callback-based api just fine, but I don't want to.

malinoff commented 7 years ago

@dabeaz

How does this modified class intersect with the general usage of Curio?

As a regular user, I shouldn't even notice a custom Task class is used. As a library developer, I have to provide a Task which is fully compatible with curio.Task and it's my responsibility to stay compatible between releases.

Is the spawn() method simply modified to return this new class?

Yes.

Is it used elsewhere?

Right now - no, it's not. But I just started to explore ways of how I could implement a curio adapter; I may need something else in future.

I'm going to fork curio, implement the ability to specify a custom Task class and try to make my functional tests pass. I'll see what else is needed.

njsmith commented 7 years ago

@njsmith AMQP is asynchronous (unlike HTTP). Various events may (or may not) happen at some point in future, when the broker decides to reply back (or not), and there have to be a way to respond to such delayed events.

You're thinking of HTTP/1.1. HTTP/2 is asynchronous in exactly the same way as AMQP. That's why I pointed to hyper-h2 instead of h11 :-).

The use of futures is hidden from the user's point of view - I could provide a callback-based api just fine, but I don't want to.

hyper-h2 doesn't use either callbacks or futures. It's also mature, widely used, and pretty much the canonical example of a sans-io library. I'm not the terminology police, but I'd actually hesitate to call a library using callbacks/futures "sans-io".

malinoff commented 7 years ago

@njsmith could you please point me to an existing mature h2 asyncio adapter that does not suffer from three bugs you kindly described?

njsmith commented 7 years ago

@malinoff Possibly @lukasa or someone could help you with that – I don't really follow h2/asyncio development that closely. There's nothing in h2's design that makes it intrinsically prone to those bugs though; they're really asyncio issues.

Lukasa commented 7 years ago

@malinoff As @njsmith said, h2 isn't prone to those bugs because it doesn't interact with those systems. It can't fail to propagate backpressure because it doesn't own the source of bytes: the asyncio glue code does. h2 will buffer, if you ask it to, but it doesn't have to buffer any more than one complete HTTP/2 frame, because again the caller is responsible for feeding in the bytes and triggering the consumption of them. And finally, because h2 owns no sockets it cannot have problems with closing the event loop before the socket is drained.

All of these problems are in asyncio and the asyncio glue code. A trio/curio adapter to h2 would happily avoid them. There is no need for callbacks: all callbacks are doing is providing you with some implicit state management. By all means feel free to use Future objects, they're a perfectly cromulent abstraction, but your AMQP library could quite easily avoid both them and callbacks if you wanted.

malinoff commented 7 years ago

@njsmith @Lukasa I never said that h2 is prone to those bugs, actually. I explicitly asked for an adapter for asyncio that isn't prone to those bugs, simply because I can't find one (at least after a couple of hours of googling). Anyway, by no means I am a professional python developer, and I'm certainly willing to learn from the greatest. If you have time (and interest, of course), could we please discuss the possibility of removing the use of Future objects in my amqp client? I've opened the issue.

Lukasa commented 7 years ago

I explicitly asked for an adapter for asyncio that isn't prone to those bugs, simply because I can't find one (at least after a couple of hours of googling).

So I suspect such a thing is difficult to find, per @njsmith's comments about asyncio itself. I can give you a Twisted adapter that resolves 1 and 2, but not anything for asyncio.

Anyway, I'll happily leap in to your issue and talk about Futures. As I said above, Futures certainly can be part of a good sans-io design: they just don't have to be. :smile:

dabeaz commented 7 years ago

Just thinking about this further, it seems kind of weird to me that a Task would be used as a Future. Normally, the return value of a Task (i.e., returned by join()) is the final return value of a coroutine. A set_result() method seems like a strange out-of-band way to set a result that's different from this. If anything, it exists in conflict with it. I think I'd probably need to see a concrete example of what's being attempted with it.

Independently of that, there are still probably scenarios where having a custom Task class could be useful. Although, I wonder if merely down-classing an instance would be sufficient:

class MyTask(Task):
       ...

 task = await spawn(coro)
 task.__class__ = MyTask
malinoff commented 7 years ago

@dabeaz I've stripped out Futures from the core of my library, so that's not an issue anymore.