WIP: channel: give the channel to the callback

[dzen]

this allows the consumer to ack message from the coroutine

This is a sample on how we could correctly consume message.

44

[dzen]

@mpaolini travis integration seemed okay :)

[mpaolini]

@dzen great stuff, thanks!

[dzen]

hello @mpaolini,

I tried to fix tests. I will also add some more documentaiton. would you please test it ?

Thanks :)

[mpaolini]

hello @dzen I'm sorry the past few weeks and the next few ones I haven't had time to check this.... Will definitely try sometime soon

[dzen]

@smurfix Did you test this PR ? Any though on it ?

thanks.

[smurfix]

The problem I have with this patch is that the Future async() call which starts the callback is not stored anywhere. That's bad practice. If I want to log errors by my callbacks, or in case I have to take the whole channel doen because of some problem (which implies canceling all the running callbacks – think "the callbacks are talking to a remote server which just crashed" as one example), I'd have to add a wrapper to all of my callbacks instead of tackling the problem at its origin.

A callback should be able to run its job within another task, but it shouldn't be forced to do that. If all the callback does is to set_result() a future or simething equally cheap and non-blocking, running it in its own task is overhead I'd rather avoid.

WRT the actual semantics (the new parameter), I've never had the problem the patch is trying to solve because all my callbacks are methods and the channel is part of the object anyway, but I still can see the appeal of doing something like that.

Personally I wouldn't add a new parameter (it's an major incompatible change after all). Instead, I'd simply attach the channel to the envelope object. Indeed, as soon as we have that, we could simply add basic_ack() and basic_nack() methods to the envelope for added convenience

[dzen]

Yes, this patch doesn't keep the running task somewhere, therefore we can't get its results or its exception. This is a bad thing.

aioamqp is in its early versions and I prefer to make function definition changes now before we get to a major versions.

We have to take some decisions about the design of the current API. we will review your suggestions !

[dzen]

Hello again.

The task is not necessary. Moreover:

• Errors are lost in the task
• Its a huge overhead to add tasks to the eventloop
• Its hard to manage on errors.

@smurfix I added a commit to remove the task, and restored the yield from on the coroutine.

[dzen]

I'm gonna merge this and create a new release.

[dzen]

Merged.