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

Errback api #363

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@meejah
Member

meejah commented Mar 16, 2015

What do you think of this, as an idea?

I'm not sure about the "_create_future()" API, and perhaps an interface would be nice for "the thing we give to errbacks" (i.e. Failure or AsyncioFailure).

I'm hopeful there's a better way to get asyncio to do an event-loop iteration, or call all completed Futures or whatever. But for now at least all the error-case tests can run in both asyncio and Twisted.

meejah added some commits Mar 16, 2015

Add a _create_failure API, and an asyncio compatibility object
This allows us to use the same errback() API between Twisted
and asyncio, and also create Failure/AsyncioFailure objects
via the FutureMixin API
hack error-case tests to work with asyncio/trollius too
Twisted is nice enough to call your callback/errback immediately
if a Deferred already has a result.

asyncio does not do this, requiring a trip through the event-loop.

I also can't find any test-runner support for asyncio Futures,
so even if we "properly" yielded control that wouldn't help either.
@oberstet

This comment has been minimized.

Member

oberstet commented Mar 17, 2015

@meejah Thanks for splitting this out into a separate PR! I have been thinking about this. Please be prepared for more discussions;) You might wonder why I'm so reluctant and not merge such a relatively straight-forward PR right away .. I will try to explain why.

What we do actually do here is come up with a mixin that allows one to write idiomatic code that runs on both Twisted and asyncio: source code compatibility.

The uniform handling of failures (and hence the need for a asyncio analog to Twisted's Failure) is just one aspect. Regarding this specific latter aspect, I am wondering what the asyncio guys think about on the mailing list (python-tulip).

Further, the current classes in AutobahnPython are somewhat misplaced/misdesigned for the general task above anyway:

#242
#243

For these reasons, I think it would be great if we took the chance and come up with a solution that actually solves #242 and #243 too.

As this could be of use not only in AutobahnPython, I have created: https://github.com/tavendo/txaio

AutobahnPython currently only depends on six. With above, the 2nd dependency would be txaio.

So the task would be: write txaio and adjust the AutobahnPython code to using that. What do you think? Makes sense?

@meejah

This comment has been minimized.

Member

meejah commented Mar 17, 2015

No worries, I understand why you want to get this right.

In the end, for Twisted, it provides really nice default logging if we can call log.err(failure, informative_message). To do anything remotely similar with asyncio we need the three args from sys.exc_info() (from what I understand) so that we could make up log messages with a stack-trace (if desired).

I'll have to look into tulip/asyncio some more to get a better handle how this sort of thing is "normally" done in callbacks.

The other piece (that I basically just hacked in "something" in this PR) is the testing aspect. I can't find any information about the/"a" recommended way to "wait" for your Future in a test (i.e. like returning a Defered to trial or pytest-twisted). In the tests I wrote, all the Futures/Deferreds are for sure immediately available, but you still need to get asyncio to "wait" once.

What might be worthwhile -- perhaps as part of txaio -- would be a pytest plugin or similar (just a utility method?) that makes test-writing a little easier with less if statements ;)

@meejah

This comment has been minimized.

Member

meejah commented Mar 17, 2015

p.s. in the meantime, do you think it's worth making a (somewhat smaller) PR that at least fixes the things that definitely won't work in protocol.py + asyncio? I think there's only two:

...but I guess also, onUserError will "fail" for the cases where it's called from an errback (traceback.print_exc() will be sad if there's no current exception). Not fatal, but it won't print anything.

@meejah

This comment has been minimized.

Member

meejah commented Mar 17, 2015

Additionally, instead of a mixin-style class (requiring inheritence, even though they're all static methods), what about just having txaio provide the API methods? Then at import-time txaio determines whether to use the Twisted or asyncio (or Trollius) "real" methods.

So client code (e.g. in Autobahn) would end up looking something like:

   import txaio

   d = txaio.create_future()
   ...
   txaio.resolve_future(d, "yay")

   class Something(object):
       def do_a_thing(self):
            return txaio.create_future_success("it worked")

...etc. This would allow users of txaio decide whether they wanted to use inheritence or composition or bare methods or whatever to accomplish their task. What do you think?

(cc @oberstet @hawkowl)

@oberstet

This comment has been minimized.

Member

oberstet commented Mar 17, 2015

@meejah Ah, I like it. Only exposing simple functions in txaio, and let users choose.

I think users should be able to actively select Twisted vs asyncio, even when both are available.

from txaio import tx as txaio

vs

from txaio import aio as txaio

or something like it.

We still would use these functions then in AutobahnPython to create a (private) mixin class?

Since the reason the current code looks like it does is because I was looking for a solution with the following.

Consider

https://github.com/tavendo/AutobahnPython/blob/master/autobahn/wamp/protocol.py#L1004

Essentially, the code there should run on both Twisted and asyncio. The concrete classes for ApplicationSession for Twisted and asyncio then mix in the respective FutureMixin, which provides

https://github.com/tavendo/AutobahnPython/blob/master/autobahn/twisted/wamp.py#L62
https://github.com/tavendo/AutobahnPython/blob/master/autobahn/asyncio/wamp.py#L59

The user then imports the concrete class

from autobahn.wamp.twisted import ApplicationSession
from autobahn.wamp.asyncio import ApplicationSession

Or is there a different solution?

@meejah

This comment has been minimized.

Member

meejah commented Mar 18, 2015

We could use txaio via mixins like currently (that call-through to txaio) , but I was thinking along the lines that any code like self._create_future() would simply become txaio.create_future() and there'd be no need for mixins.

Users would "select" asyncio/trollius/twisted by installing them. As in: if you have Twisted, you use it; else try asyncio; else try trollius; else fail.

That is, import txaio would basically just try to import things until something works. If "user selects what they want, even though they have asyncio AND Twisted around" is a use-case I'll think a little more on it :) but that could be as simple as your "import ... as" examples above.

So far the only hard part is trying to support the nice options Twisted has for "DeferredList" and gather. I might split this API so its simpler code, and more-explicit/easier to understand from the user level.

@hawkowl

This comment has been minimized.

Contributor

hawkowl commented Mar 18, 2015

There's a case where Twisted and asyncio will be available - if you have Twisted installed on py3, the majority of core is ported. So, you'd need to be explicit in which one to choose.

@oberstet

This comment has been minimized.

Member

oberstet commented Mar 18, 2015

Here are the requirements that come to mind:

  • User must be able to chose between Twisted and asyncio when both are installed.
  • As far as Autobahn is concerned, for compability reasons, this has to be done by importing the respective classes in user code, like
from autobahn.wamp.twisted import ApplicationSession
  • within Autobahn itself, we want as much code as possible written in a language agnostic way

I currently don't see how we can achieve above without mixin classes - but I am open to other ideas.

If we do not find another solution (other than mixins), those event loop mixin classes should reside in txaio not Autobahn, since txaio is supposed to help exactly in the general situation where one wants to write code once that runs on both on Twisted and asyncio.

In other words, the USP of txaio is: "A helper library for writing code that runs on both Twisted and asyncio." With this USP, I guess it would make sense to gather feedback early on with devs on tulip/twisted mailing lists.

@sehoffmann

This comment has been minimized.

Contributor

sehoffmann commented Mar 18, 2015

I think it should be possible to let the entry point into Autobahn (ApplicationRunner if I recall correctly) to choose the underlying framework based on the passed reactor/eventloop (if its a reacton -> twisted, if its an eventloop -> asyncio). Classes which then need framework-independent functionality could be implemented by using dependency injection (which the entry point sets up). As Autobahn already is heavily build around IoC, dependency injection would fit pretty well into it IMHO.

We could realize dependency injection by utilizing constructors or by directly modifying the object instance (session.futureHelper = foo) or by providing a dedicated interface (session.inject("futureHelper", foo). Probably there also are already a bunch of python libraries available for dependency injection.

@meejah

This comment has been minimized.

Member

meejah commented Mar 18, 2015

The only non-mixin solution I can think of would be this:

autobahn.wamp.twisted or autobahn.wamp.asyncio are the only spots where anything is imported from txaio. The respective ApplicationSession classes do something like:

    from txaio import tx as _txaio
    class ApplicationSession(...):
         txaio = _txaio

...and then protocol.py code looks like self.txaio.create_future() etc. In the end, it might cover more use cases to simply put mixins in txaio (so you can do txaio.create_future() or do a mixin-style thing like Autobahn). Then the code would look basically like it does now:

    from txaio.tx import LoopMixin
    class ApplicationSession(LoopMixin, ...):

I have a "working" PoC in my fork of txaio. I still need to add some docs and clean up the asyncio testing/waiting and probably some other things. gather_futures is kind of a horror-show, and I took a stab at supporting an inlineCallbacks/coroutine equivalent decorator (txaio.future_generator) but I'm not sure I got that right -- plus it needs PY3-only code (yield from). On the upside, the tests I do have pass on py2.7 + py3.3 on both asyncio and twisted...

There's one change from what I copied out of Autobahn: we weren't supporting callback return-value mutations (like Twisted does) in asyncio. I don't believe Autobahn depended on this behavior, but I changed the asyncio implementation to support this but it uses one asyncio internal, setting future._result if a callback returns a new value. Twisted also supports jumping to the callback chain from the errback chain (and vice-versa) and that isn't at all supported. I'm not sure of a really "good" solution here: either you get inconsistent behavior depending on the backend you select, or I keep code to make either asyncio or Twisted conform to one way or the other. Probably this is one for "feedback from twisted/tulip mailing lists" :)

@meejah

This comment has been minimized.

Member

meejah commented Mar 19, 2015

Here's how the above looks, as an idea.
All the tests pass (but there are still a few uncovered lines).

https://github.com/tavendo/AutobahnPython/compare/master...meejah:use-txaio?expand=1

@oberstet oberstet referenced this pull request Mar 19, 2015

Closed

API ideas #1

@meejah

This comment has been minimized.

Member

meejah commented Mar 23, 2015

https://github.com/tavendo/AutobahnPython/compare/master...meejah:use-txaio?expand=1

is now based off the simplified txaio, just using import txaio. There are at least some tests of creating and cancelling timed-calls (but certainly not all).

@oberstet oberstet closed this Apr 14, 2015

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