Add extractCurrentResult() and observeCurrentResult() to synchronously interact with a Deferred's result. #590

dstufft · 2016-11-11T19:43:24Z

Ticket: https://twistedmatrix.com/trac/ticket/8900

dstufft · 2016-11-12T01:36:18Z

src/twisted/internet/defer.py

@@ -52,6 +52,12 @@ class TimeoutError(Exception):
    """


+class InvalidStateError(Exception):


This should maybe derive from BaseException instead so that a:

try: undefer(d) except Exception: pass

Does not catch an invalid undefer() call the same as an exception that came from the callbacks internal to it.

Another thought about why this should come from BaseException:

There's not really much recovering from this. At best maybe you can run it through the event loop or synchronously drive it and then pass it through again... but if you can do that why didn't you do it in the first place?

rodrigc · 2016-11-12T04:31:08Z

(1) Patch needs a topfile news fragment, as specified here: https://twistedmatrix.com/trac/wiki/TwistedDevelopment#SubmittingaPatch

(2) Patch needs to modify the tests to increase coverage. If you install the Codecov browser plugin
and look at https://github.com/twisted/twisted/pull/590/files , this patch only has 16% code coverage

dstufft · 2016-11-12T16:30:52Z

@rodrigc Yea I know, I was hoping to get some feedback on the idea and implementation before I did that :)

codecov-io · 2016-11-13T14:53:20Z

Current coverage is 90.18% (diff: 100%)

Merging #590 into trunk will decrease coverage by 1.07%

@@              trunk       #590   diff @@
==========================================
  Files           843        843           
  Lines        147134     147265    +131   
  Methods           0          0           
  Messages          0          0           
  Branches      13016      13022      +6   
==========================================
- Hits         134280     132817   -1463   
- Misses        10621      12177   +1556   
- Partials       2233       2271     +38

Powered by Codecov. Last update d34091c...a4aae36

dstufft · 2016-11-13T16:09:22Z

src/twisted/internet/defer.py

@@ -159,6 +165,65 @@ def maybeDeferred(f, *args, **kw):
        return succeed(result)


+def unbox(deferred, raises=True, passthrough=True):


I'm not sure about the passthrough argument here.

Essentially, the problem boils down to if you don't have passthrough=False then when the Deferred gets garbage collected if it was in a failure state then it prints a spurious traceback to stdout. This sees to be a problem that other utilities like gatherResults has had and they have a consumeErrors parameter which defaults to False but is documented that you should almost always use consumeErrors=True.

So I guess the question is, what behavior with results makes sense?

Always pass through results, success or failure and if someone wants something different then they need to add their own callback that throws away the result.

Always pass through successful results but consume any failure results under the assumption that if you've unboxed the Deferred then you've handled the failure (and if not, you can add your own callback that re-inserts it).

Never pass through any result and "break" the callback chain if someone continues to re-use the Deferred instance after you've unboxed it. If you want that to continue working then you'll need to add your own callback that re-inserts the result.

Add a passthrough parameter that controls in both the success and the failure case whether or not it should be passed through or not.

If this is the answer, should we pass through results (success and failure) by default or not?

Add a consumeErrors parameter that controls whether or not unbox() will consume errors.

Should this default to False as the other utility functions do (though I think they do for backwards compatibility) or should this default to True to more likely do the right thing?

Should we always return success results then or should we always consume them? Does it make sense to add a consumeSuccesses parameter for success results? If so what should it default to?

After thinking about this more, I think the right answer is one of:

Always pass through successful responses and always consume errors.

Always pass through successful responses and add a consumeErrors parameter.

Defaulting to True to do the right thing by default.

Defaulting to False to match gatherResults and friends.

Always consume everything.

The reasoning for this is that there's very little downside to passing through a successful result. At worst it will just hold a reference to it until the Deferred itself gets garbage collected. So it just always passes that through.

However, if you don't consume errors it prints spurious error results to stdout so there is an incentive to consume those and not pass them through. Adding a consumeErrors parameter aligns with other utilities and allows control on this.

The downside is errors and success ends up being treated differently although that could possibly be solved by consuming everything and treating unbox() as something you should generally only call on a Deferred that is no longer going to be kept around or used.

My intuition here is that:

def unbox(deferred, raises=True, consumeErrors=True): ...

Is the "right" way forward, but I'm not as versed in what is normal in Twisted land.

So I think the behavior I expected before reading this (even though I'm no deferred expert) is to always raise, and always passthrough.

Someone who wanted raise=False can try/except and someone who wanted specifically the failure and not the exception sounds like they'll be unhappy but I think that's the "fault" of the interaction between failures and exceptions, not of your code here -- i.e., if that was a common use case, I'd expect to see an inverted failure object which is an exception that carries around the original failure.

As for passthrough which it sounds like is more important to address -- I'd have expected always passthrough for a bunch of reasons, some dogmatic and some practical

I'm discarding the asymmetric options immediately because I'd find it surprising to treat failures differently than successes personally

no side effects always being better than side effects

the typical use case for me is like having a webapp which passes deferreds all around, and then I have some way of synchronizing those, but other internals there like "log deferred errors" still might be addErrbacking the deferred I get my hands on, and I'd still want those to work

The reason I added raise=False was when I went to implement successResultOf, failureResultOf, and assertNoResult in terms it was hard to replicate the behavior of those functions without it:

successResultOf uses the failure's traceback printing mechanisms which seem to be subtly different than Python's built in ones. If we're happy with the output on failure changing slightly then this one isn't a problem.

failureResultOf returns the failure, which is impossible to do if unbox() doesn't provide that information to it.

failureResultOf when there are expected exception types has the same problem as sucessResultOf with the failure message relying on details of failure's traceback formatting.

assertNoResult wants to silence failures but not successes, so the current passthrough parameter doesn't help it here, but a consumeErrors would.

In my own personal use cases I'm hoping to use the unbox() function in functions that can either be driven synchronously or asynchronously depending on what "fetcher" is passed into them (either sync or async). I want to just always return Deferred from these functions but I need a way to hide the fact that Deferreds are involved at all for end users who want to call the code synchronously. The expected way for this to look is something like (simplified):

import requests import treq def sync_fetcher(url): resp = requests.get(url) resp.raise_for_status() return resp.content def async_fetcher(url): d = treq.get(url) d.addCallback(treq.content) return d def api(thing, fetcher): d = Deferred() d.addCallback(fetcher) d.callback("https://example.com/api/{}".format(thing)) return d # When being used from async code d = api("my-thing", fetcher=async_fetcher) d.addCallback(print) # When being used from sync code print(unbox(api("my-thing", fetcher=sync_fetcher)))

The goal is to essentially treat the unbox() function sort of like string encoding/decoding in that it turns the code into "normal" sync like code that hides the fact Deferreds are in play. However, if errors don't get consumed then the above code is going to print a spurious error to stdout when the Deferred gets garbage collected. The way to work around this would be:

d = api("my-thing", fetcher=sync_fetcher) try: print(unbox(d)) finally: d.addErrBack(lambda _: None)

Which fails at my goal of being able to tell people with "normal" sync looking code to just wrap the Deferred returning functions with an unbox() and forget about it.

Julian · 2016-11-13T16:14:48Z

src/twisted/internet/defer.py

@@ -52,6 +52,12 @@ class TimeoutError(Exception):
    """


+class InvalidStateError(BaseException):


The name here sounds a bit too general / risky to have it start being used by other things if all it currently means is e.g. UnboxingWithoutResult

Ok, I'll switch it to UnboxingWithoutResult.

Julian · 2016-11-13T16:15:22Z

src/twisted/internet/defer.py

+    Unbox a L{Deferred} that has already been fully processed.
+
+    Take an already processed (i.e. no longer waiting on any callbacks to be
+    fired) and unbox it. If the result is a L{Failure}, then it will re-raise


Missing a word here (deferred), and I think it'd be a bit nicer to flip the order of the english sentence so that you talk about the happy normal case first, and then "it reraises" is your otherwise at the end.

Julian · 2016-11-13T16:16:15Z

src/twisted/internet/defer.py

@@ -159,6 +165,65 @@ def maybeDeferred(f, *args, **kw):
        return succeed(result)


+def unbox(deferred, raises=True, passthrough=True):
+    """
+    Unbox a L{Deferred} that has already been fully processed.


Do other places use the lingo "fully processed", I'm not experienced enough to know, but I think common vernacular if that's what you mean is "that has already fired its result" or the like.

I wasn't completely happy with the "has already fired its result" because of the behavior of:

d = Deferred() d2 = Deferred() d.addCallback(lambda _: d2) d.callback(None)

Has the Deferred d "fired" its result? It's had callback() called on it so it seems to me like it had, but it's been chained to another Deferred that has not yet fired its result. Ultimately I don't really care much what the terminology is, I was just worried about that being confusing.

Julian · 2016-11-13T16:18:13Z

src/twisted/internet/defer.py

+    B{This function CANNOT take a Deferred with pending asynchronous work to be
+    done and make it synchronous. It can ONLY unbox a Deferred which already
+    has successfully completed.}
+


I think even if you don't borrow the existing terminology that at least linking to successResultOf might be a nice crosslink.

Julian · 2016-11-13T16:19:44Z

src/twisted/test/test_defer.py

@@ -645,6 +645,121 @@ def test_cancelGatherResultsWithAllDeferredsCallback(self):
        self.assertEqual(callbackResult[1], "Callback Result")


+    def test_unbox(self):
+        """
+        L{defer.unbox} should unbox a Deferred that has already had it's


Julian · 2016-11-13T16:48:47Z

src/twisted/internet/defer.py

@@ -159,6 +165,65 @@ def maybeDeferred(f, *args, **kw):
        return succeed(result)


+def unbox(deferred, raises=True, passthrough=True):


So I think the behavior I expected before reading this (even though I'm no deferred expert) is to always raise, and always passthrough.

Someone who wanted raise=False can try/except and someone who wanted specifically the failure and not the exception sounds like they'll be unhappy but I think that's the "fault" of the interaction between failures and exceptions, not of your code here -- i.e., if that was a common use case, I'd expect to see an inverted failure object which is an exception that carries around the original failure.

As for passthrough which it sounds like is more important to address -- I'd have expected always passthrough for a bunch of reasons, some dogmatic and some practical

I'm discarding the asymmetric options immediately because I'd find it surprising to treat failures differently than successes personally

no side effects always being better than side effects

the typical use case for me is like having a webapp which passes deferreds all around, and then I have some way of synchronizing those, but other internals there like "log deferred errors" still might be addErrbacking the deferred I get my hands on, and I'd still want those to work

Julian · 2016-11-13T16:51:08Z

src/twisted/internet/defer.py

+    else:
+        [result] = results
+
+        if raises and isinstance(result, failure.Failure):


I think you'll run into trouble here if you have a function that callbacks with a failure (but maybe that will already fail in a place you don't control).

But like, doing:

result, failure = [], []

and then addCallback / addErrback would let you distinguish "properly".

Also I think you need tests both for this specifically and for the BaseException vs. Exception distinction (unless I missed an existing one for the latter).

I've added a test for the BaseException versus Exception distinction. However to the best I can tell Deferred itself treats Deferred().errback() the exact same as Deferred().callback() other than constructing the original Failure instance that will be passed through the chain.

IOW, it's impossible to have a Deferred that returns a Failure.

Julian · 2016-11-13T16:51:21Z

src/twisted/test/test_defer.py

+
+    def test_unboxRaisesFailureException(self):
+        """
+        L{defer.unbox} should raise the original exception if the result of the


s/should raise/raises (and same with below)

I think generally AIUI we try to follow https://jml.io/pages/test-docstrings.html

Julian · 2016-11-13T16:54:14Z

src/twisted/test/test_defer.py

+        """
+        L{defer.unbox} should return the failure instance if raises=False.
+        """
+        d = defer.Deferred()


Might solve your initial motivation to add passthrough if you def cbDeferred(self): d = Deferred(); d.callback(1234); return d and def ebDeferred(self, exception): d = Deferred(); d.addErrback(lambda _: None); d.errback(exception); return d and did your lambda _: None there in the latter so you didn't have to keep repeating it.

Julian · 2016-11-13T16:54:53Z

src/twisted/topfiles/8900.feature

@@ -0,0 +1,2 @@
+Added twisted.internet.defer.unbox function to enable the unboxing of a
+"resolved" Deferred.


Seems like yet a 3rd language here on what we call "a deferred that's already fired".

Julian · 2016-11-13T17:52:44Z

Just putting this here for full context, some stream of consciousness discussion from IRC:

 12:34:37        tos9| dstufft: OK now maybe back to IRC would be helpful just for back and forth :/
 12:34:50        tos9| dstufft: Your use case looks similar to mine, but also a bit odd to me
 12:34:59  dstufft| what's the difference between d.addCallbacks(cb, eb) and d.addCallback(cb); d.addErrback(eb)
 12:35:01  dstufft| is there one?
 12:35:10        tos9| dstufft: The latter will catch even errors in the callback function
 12:35:42        tos9| dstufft: You want to hide users from deferreds, but still tell them they need to use a black box function to unbox them?
 12:36:25        tos9| One they especially have to write `from deferreds import unbox` or whatever to use?
 12:38:16     dstufft| tos9: well by "hide" I don't mean "make it so they never ever have to know that Deferreds are here" but more like "give a simple function they can use 
                       to avoid having to learn about Deferred or munge with them". This is so I don't have to add a foo() and a sync_foo() method to my API
 12:38:34        tos9| OK
 12:38:57        tos9| dstufft: Wouldn't it be even better though if your synchronous API had a way to inject things into your async API that handled the unboxing?
 12:39:04        tos9| and just plain not return a deferred in that case?
 12:39:55        tos9| like right now you're injecting a thing which you "know" makes all your deferred synchronous, but then still bubbling them up to people
 12:40:34        tos9| that kind of messes with your return types though. bleh. Sorry, my brain is still catching up.
 12:41:13        tos9| dstufft: (I have pretty much exactly your broad use case by the way, I have two clients, one using treq and one using requests)
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 12:42:42     dstufft| tos9: Yea, this is for a thing in pip (well in packaging, but consumed by pip), and in pip I might just start threading Deferred throughout the code 
                       base even in the sync case (Which is all pip is going to be for now, full out sync, no event loop) but I want this library to be re-usable for people 
                       too, but I'm not like, super worried about telling sync users if they don't want to deal with the Deferred then unbox them
 12:43:52  dstufft| but if they have to mess around adding callbacks or errbacks to get a "normal" workflow where unbox() raises an error and doesn't spew errors on stdout 
                    even if they've been raised, then I'll need to either add my own unbox function that does the callbacks/errbacks for them, or add sync_foo
 12:45:07          * | tos9 nods
 12:45:29        tos9| dstufft: The thing that's bugging me is I'm still trying to decide whether your use case is exactly mine or not
 12:45:47        tos9| Because right now I'm basically treating my synchronization as plucking (peeking at) a deferred in its normal flow
 12:46:02        tos9| And the only thing that seems fairly "obvious" to me is that unbox should take exactly one of those two strategies
 12:46:15        tos9| either it's "peek at a deferred synchronously" or it's "end a deferred synchronously"
 12:46:42        tos9| but the two of us are taking opposite strategies :), so I'm trying to decide if you're right and I should change mine.
 12:46:46  dstufft| Yea, that's how I originally wrote unbox because it seemed like a simple enough thing to just return the result from the callback so that the deferred 
                    wasn't "ended" 
 12:47:01  dstufft| but ending is how I expected it to be used
 12:47:07  dstufft| in the common case
 12:47:10  dstufft| I could be wrong though!
 12:47:51  dstufft| it wasn't until I discovered that simply passing the failure through on errback did the "spew to stdout" thing :/
 12:49:12        tos9| dstufft: I think I have to ponder more to form an opinion that I'm confident -- or maybe someone else will weigh in
 12:49:23        tos9| maybe I'll try to rereview later tonight if that's OK and I have some more clarity?
 12:49:30     dstufft| tos9: that's fine :)
 12:49:36  dstufft| I'm fixing up the other things now
 12:50:08        tos9| k cool. Gonna also paste this whole thing into the ticket even though it's stream of consciousness just so that the full context is there

dstufft · 2016-11-13T18:28:12Z

Github hid the original comment, but I think the main open questions are around raising the exception versus returning the failure and how to handle passthrough. You can see the original comments and follow up comments at #590 (review).

exarkun · 2016-11-14T00:47:47Z

This is a very frequently requested feature. It's been turned down many times in the past. It'd be nice to see some discussion here of why it makes sense to add it now when it never did before.

dstufft · 2016-11-14T01:09:45Z

@exarkun Do you have any link to relevant discussions in the past? I tried searching the bug tracker but couldn't find any tickets open or otherwise but I don't think there is an obvious terminology for what this is called to search by. Since I don't have any context about why it was turned down previously I can't make an argument about why it makes sense now when it never did before.

I can tell you what my use case is though (and I did earlier, though I'm happy to reiterate), and @Julian had a similar use case. I also see some similar looking code in https://github.com/twisted/tubes/blob/master/tubes/undefer.py though not exactly the same since there is a yield skip in the middle of what would be the unbox() function here. I don't know enough about tubes to know if you could move that earlier or not. I've also been able to replace some other parts of Twisted using the unbox() function, so the idiom has been used at least once or twice inside of Twisted itself.

exarkun · 2016-11-14T01:45:12Z

I don't have any links, sorry. I'm sure there is some discussion in the twisted-python archives but it's pretty hard to find anything there.

The gist of the argument against this functionality is that in a normal program, the only way you know a Deferred has a result for you is if you are a callback on that Deferred and you have just been called. In that case, you don't need this "unbox" function because the result is passed to you as an argument.

Any other time you might call such an "unbox" API, you have no idea if the Deferred has a result for you or not. The only way you could use this API with any chance of success is to poll "unbox" and handle the "UnboxingWithoutResult" you get repeatedly.

@radix wrote https://github.com/radix/synchronous-deferred to solve the kind of problem issue 8900 described. Apparently from his experience with that he decided any effect-like library is a better approach to the problem.

Twisted employs this give-me-the-result pattern in trial to help write tests where you actually can know when there's a result (or if you're wrong, an exception is great, then your test can fail). I don't recall any other places it's used, I'd be interested to look at them if you point them out.

The yield skip in the tubes code is important where it is. That function is effectively just translating between two different callback APIs. It's not reaching in to a Deferred for its result at a particular point: it's waiting for a result by adding a callback.

dstufft · 2016-11-14T01:51:03Z

@exarkun For my use case, I'm using Deferred but there's no event loop or anything it's waiting on. The callbacks are just doing blocking IO inside of them. The reason for this is so that people can swap out the one place that IO is done at runtime with something that returns a Deferred itself and uses Twisted's or asyncio's event loop instead of just directly doing blocking I/O and have the thing just work.

In my use case I can pretty clearly say that either the Deferred has a result for you always at that point in time or you should not be using unbox() because you're being driven via an event loop.

exarkun · 2016-11-14T12:53:24Z

If they write code that uses unbox then they cannot swap out the implementation for one that uses an event loop. If they do so, their unbox-calling code will suddenly start failing.

dstufft · 2016-11-14T12:57:53Z

@exarkun Right, but my library will work both ways. They can either use it synchronously with unbox or asynchronously without by passing in a a sync or async implementation. They can even slowly start pulling the unbox further up the call stack to progressively convert more of the program to be Deferred supporting before adding an event loop.

exarkun · 2016-11-14T13:05:46Z

It doesn't seem convincing that, because the API can be used with some Deferreds from one library when used in one mode, the API should be added to Twisted so it is available for use with all Deferreds.

Maybe you should put this API in your library? Then it's close at hand for the audience to which it is relevant.

dstufft · 2016-11-14T13:08:02Z

@exarkun If I have to do something like that I might, though if this keeps coming up and there are at least two people in the last day or two alone that have need for it, it seems like having multiple copies of this function in different libraries and applications is less than ideal.

Julian · 2016-11-14T13:14:48Z

I'll admit I haven't yet wrapped my head entirely around @dstufft's use case, mostly because I'm so deep in my own (which only happened to touch on this since I did implement it locally as a one off) but --

@exarkun it does happen to be that my own use case is in testing, but not in a place where I have access to a test case per se (I have a testing helper, and a lack of desire to write code that looks like Helper(test_case=self) just so I can access functionality I want "globally"). I.e., I literally want to call successResultOf, but I have no test case, and if in fact my assumption is wrong (that I somehow end up with a deferred that hasn't fired) I do want the exception. That was why I felt it was compelling enough to review this, was that to me it doesn't feel very much like creating something new, it's what's there, just without asking me to have a test case around.

I do obviously believe your skepticism though and again I haven't properly understood @dstufft's use case myself yet -- does it make you feel any less skeptical if this stayed in twisted.trial?

exarkun · 2016-11-14T13:21:05Z

As a test helper, I'm believe it's useful (at least, I don't know of a better approach). I'm pretty sure I'm even the one who committed successResultOf, failureResultOf, and assertNoResult to trial? I reviewed the code, at least. I don't have any problem with making these APIs more easily used in test code. In general, it seems like at this point the pyunit inheritance-based approach to providing testing tools should have been proven suboptimal and we've only failed to move on to something better due to inertia.

Julian · 2016-11-14T13:25:02Z

@exarkun that was a leading question because I knew you were the one :P

And yep full agreement on the unittest thing.

@dstufft how dirty does it make you feel to say what we're actually doing is improving the testing helpers, and then if you're really convinced that your API is how it should be, that you just use those in real code illicitly :D? Sorry for the runaround, it's what happens when you have to deal with someone [me] who's not really an expert in twisted lore (cough).

dstufft · 2016-11-14T13:28:12Z

@Julian Well, if it's not included in what @hawkowl splits out into a separate deferred library then I can't use it in the main consumers of my library anyways (pip and setuptools) so I'll probably just add it to my library or skip the idea of using Deferred as my base primitive there all together.

Julian · 2016-11-14T13:37:40Z

That sounds like a separate question to me -- I don't know what @hawkowl's plans are, but I'd assume / hope that successResultOf and friends would be part of a split-off-Deferred's test helpers too.

Julian · 2016-11-14T13:39:10Z

(FWIW personally I think the right next step rather than yanking your chain and asking for work that may or may not benefit you is for someone to actually understand your use case specifically and then make a call on whether it sounds like this is the right thing to add for it, but I still haven't done so, sorry :/)

dstufft · 2016-11-14T14:01:04Z

This started with me going "Does anyone have any good patterns for making an API that can be used synchronously and asynchronously?". Then @hawkowl suggested just using Deferred for both cases and pointed me at this snippet of code:

@attr.s
class URLResponse(object):
    url = attr.ib()
    response = attr.ib()
    headers = attr.ib()

def do_something_with_content(resp):
    print("got url", resp.url)
    return resp

@inlineCallbacks
def get_async(url):
    import treq
    resp = yield treq.get(url)
    content = yield resp.content()
    return URLResponse(url=url, headers=resp.headers, content=content)

def get_sync(url):
    import requests
    resp = requests.get(url)
    return URLResponse(url=url, headers=resp.headers, content=resp.content)

def _api(url, fetcher):

    d = Deferred()
    d.addCallback(lambda _: fetcher(url))
    d.addCallback(do_something_with_content)
    d.callback(None)
    return d

def async_api(url):
    return _api(url, get_async)

def sync_api(url):
    return _api(url, get_sync)

Which is great, it does basically everything I need, except that it forces every consumer of the library to write their software using the Deferred idiom which tends to be a bit infectious since (currently) every thing you do has to be registered as a callback of the Deferred. That makes it hard or impossible to use this hypothetical API in code that isn't already using Deferred everywhere unless you have an unbox() like function available to you from somewhere (either writing it yourself as a one off, or pulling it in from somewhere else).

I'm trying to make something that both sync and async users can consume and I'm even willing to add a slight annoyance to sync users to make it happen. Currently though without something like this available (either in my own library or in Twisted or whatever) it feels like the cost of using Deferred is much higher than it's worth to make things that async users can easily consume since my main use case is sync.

I'm not sure how else to really describe my use case. To me it feels fairly obvious but I am probably too close to the problem.

thijstriemstra · 2016-11-16T01:45:30Z

src/twisted/internet/defer.py

+
+    @raise UnboxingWithoutResult: If C{deferred} has pending work to be done
+        and has no result to unbox.
+


an @SInCE marker would be useful here.

dstufft · 2016-12-01T05:23:50Z

Just a note since @glyph called it out explicitly:

This PR already has reimplemented successResultOf and failureResultOf using the extractCurrentResult function.

I've also renamed unbox() to extractCurrentResult(). I believe that this should satisfy @glyph's concerns re: the naming, and I think it better describes what it's doing than extractImmediateResult(), which could be interpreted to mean "immediately finish running the Deferred and then extract the result".

hawkowl · 2016-12-01T06:04:08Z

FWIW, I have seen 'current' result used elsewhere when referring to a Deferred.

exarkun · 2016-12-01T12:46:23Z

@glyph It would probably be good to capture some of that thought process somewhere easier to find. What are the chances future maintainers of this API are going to come read the full PR discussion?

dstufft · 2016-12-01T13:23:53Z

@exarkun Where would that get captured at? I did add a little bit to the doc string but I'm not sure if there is a better place?

exarkun · 2016-12-01T13:45:05Z

@dstufft The docstring seems like it covers the right stuff now. Part of glyph's point was that there's a bigger picture here and that's guiding certain decisions. It seems like that big picture should be capture somewhere (else, it's difficult for people who weren't in the referenced conversations to be guided by it). This feels more like some kind of "vision" doc than anything else. Maybe it's just a blog post on labs, maybe it's a wiki page.

dstufft · 2016-12-01T13:52:32Z

@exarkun Ok! I'll leave that to @glyph or someone else to write the blog post/wiki/vision documentation then :)

glyph · 2016-12-01T19:07:32Z

@exarkun

It would probably be good to capture some of that thought process somewhere easier to find. What are the chances future maintainers of this API are going to come read the full PR discussion?

Agreed. I wrote it here first because I don't know where the right place is to get this in front of maintainers. I started thinking about that but figured I'd better just get my thoughts down here given the opportunity, they can be copied and pasted elsewhere without much trouble :)

In the future, stuff like this really needs to go on the mailing list as it is discussed. (Which is totally my bad.) Especially if we have an in-person discussion that might affect the project, someone needs to write it up and give everyone a heads up. Generally we need to use the ML more, especially as cross-project discussions fracture across multiple different IRC channels. In the future I'll try to fire off a message when each discussion occurs.

I think it might be a little premature to put it somewhere official-looking like labs.tm or the docs, but a wiki page might be appropriate at this point.

glyph · 2016-12-01T19:10:21Z

As I was writing that, I realized that a simple link might do the trick. I posted to the mailing list here about this thread.

termim · 2016-12-01T19:40:52Z

There is a typo in the docstring - look for "originak"

glyph · 2016-12-09T00:46:59Z

Another thought about this PR:

With this function avoiding the foot-gun nature of extracting a synchronous result as much as possible, the ill-considered .result attribute of Deferred itself really ought to be deprecated.

dstufft · 2016-12-09T00:52:07Z

@glyph presumably that would be another PR?

glyph · 2016-12-09T01:50:43Z

Oh, yes, absolutely. Just noting it.

exarkun

Thanks! I've been meaning to get to this for a while now, sorry it took so long. I'll try to be quicker on the next iteration.

exarkun · 2017-01-21T01:38:46Z

src/twisted/internet/defer.py

@@ -53,6 +53,13 @@ class TimeoutError(Exception):



+class NoCurrentResult(BaseException):
+    """
+    This error is raised when attempting to call extractCurrentResulton a


Could be L{extractCurrentResult}. Also, missing space before the "on" here.

exarkun · 2017-01-21T01:39:31Z

src/twisted/internet/defer.py

+class NoCurrentResult(BaseException):
+    """
+    This error is raised when attempting to call extractCurrentResulton a
+    L{Deferred} that has not yet fired all of its callbacks.


Typically, we say the Deferred fires or is fired. The callbacks are just called, I think. Unless the lingo has moved on while I wasn't paying attention.

exarkun · 2017-01-21T01:40:58Z

src/twisted/internet/defer.py

+
+    Take a deferred that has already fired all of its callbacks and extract and
+    return the result of the L{Deferred}. This process of extracting the result
+    will, by default, consume the result, error or not unless the


I think ... consume the result - error or not - unless the .... Certainly not just a single comma before the "error or not", though. The aside is just "error or not" and it should be delimited on both ends by some punctuation. This could still be commas but there's already a lot of commas here so I think dashes would be less confusing.

exarkun · 2017-01-21T01:42:34Z

src/twisted/internet/defer.py

+    Take a deferred that has already fired all of its callbacks and extract and
+    return the result of the L{Deferred}. This process of extracting the result
+    will, by default, consume the result, error or not unless the
+    L{passthrough} parameter is set to L{True}. If the L{Deferred} is in an


"has an error result"? "is in an error" seems weird.

exarkun · 2017-01-21T01:43:12Z

src/twisted/internet/defer.py

+
+    The primary purpose of this function is to allow code to be written that is
+    internally asynchronous but which can be consumed without an event loop in
+    an synchronous fashion by using L{extractCurrentResult} at the boundaries.


"a synchronous ..."

exarkun · 2017-01-21T01:58:35Z

src/twisted/test/test_defer.py

+        d.callback(result)
+
+        self.assertIs(defer.extractCurrentResult(d), result)
+        self.assertIsNot(defer.extractCurrentResult(d), result)


Assert that the result becomes None perhaps? Asserting it's not the result leaves the field wide open for what the behavior actually is. I don't see any reason to let the test suite leave the intent this wide.

exarkun · 2017-01-21T01:58:45Z

src/twisted/test/test_defer.py

+        result = d.result
+
+        self.assertIs(defer.extractCurrentResult(d, raises=False), result)
+        self.assertIsNot(defer.extractCurrentResult(d), result)


Ditto comment for the non-error case.

exarkun · 2017-01-21T01:59:40Z

src/twisted/topfiles/8900.feature

@@ -0,0 +1,2 @@
+Added twisted.internet.defer.extractCurrentResult function to enable extracting


The style guide for news fragments suggests "The new function twisted.internet.defer.extractCurrentResult ..."

exarkun · 2017-01-21T02:00:38Z

src/twisted/trial/_synctest.py

-                "Success result expected on %r, found no result instead" % (
-                    deferred,))
-        elif isinstance(result[0], failure.Failure):
+                "Success result expected {0!r}, found no result "


This dropped the "on" which is important to the meaning.

exarkun · 2017-01-21T02:01:24Z

src/twisted/trial/_synctest.py

-                "found failure result instead:\n%s" % (
-                    deferred, result[0].getTraceback()))
+                "Success result expected on {0!r}, "
+                "found failure result instead:\n{1}".format(


Any particular reason to use {0} and {1} instead of just {}?

Habit for Python 2.6 compatibility.

Reimplement SynchronousTestCase.successResultOf, SynchronousTestCase.failureResultOf, and SynchronousTestCase.noResult using defer.extractCurrentResult instead of custom one off code.

dstufft · 2017-01-22T20:03:22Z

Just as a note, I've taken the action that @exarkun asked for and I've removed the passthrough=True/False parameter to extractCurrentResult.

There are now two functions, extractCurrentResult and observeCurrentResult which have pretty much the same functionality, except extractCurrentResult will set the current result of the Deferred to None and observeCurrentResult will preserve the current result. They both still take a raises parameter.

exarkun · 2017-01-23T01:16:44Z

Thanks. See #690.

dstufft commented Nov 12, 2016

View reviewed changes

dstufft mentioned this pull request Nov 13, 2016

[WIP] Implement PEP 503 pypa/packaging#87

Closed

dstufft force-pushed the 8900-dstufft-undefer branch from 67454c0 to 75fac64 Compare November 13, 2016 14:53

dstufft changed the title ~~Add undefer() to extract result from a completed Deferred~~ Add unbox() to unbox a completed Deferred Nov 13, 2016

dstufft force-pushed the 8900-dstufft-undefer branch from 75fac64 to 44ec399 Compare November 13, 2016 16:01

dstufft commented Nov 13, 2016

View reviewed changes

Julian requested changes Nov 13, 2016

View reviewed changes

dstufft force-pushed the 8900-dstufft-undefer branch 2 times, most recently from 5104d81 to b6c5c18 Compare November 13, 2016 18:24

thijstriemstra reviewed Nov 16, 2016

View reviewed changes

dstufft force-pushed the 8900-dstufft-undefer branch from 9f4b9b2 to 3f91ec5 Compare December 1, 2016 12:03

dstufft force-pushed the 8900-dstufft-undefer branch 2 times, most recently from e0be89f to 52897b8 Compare December 1, 2016 15:51

dstufft mentioned this pull request Dec 1, 2016

Add support for specifying plugins via pip.commands entry points pypa/pip#4146

Closed

dstufft force-pushed the 8900-dstufft-undefer branch 3 times, most recently from be666df to f939320 Compare December 5, 2016 01:12

exarkun requested changes Jan 21, 2017

View reviewed changes

dstufft added 2 commits January 22, 2017 14:39

Add extractCurrentResult() to unbox a completed Deferred

1722696

Use defer.extractCurrentResult in SynchronousTestCase

e1e6467

Reimplement SynchronousTestCase.successResultOf, SynchronousTestCase.failureResultOf, and SynchronousTestCase.noResult using defer.extractCurrentResult instead of custom one off code.

dstufft force-pushed the 8900-dstufft-undefer branch from f939320 to b2a120c Compare January 22, 2017 19:42

dstufft changed the title ~~Add extractCurrentResult() to unbox a completed Deferred~~ Add extractCurrentResult() and observeCurrentResult() to synchronously interact with a Deferred's result. Jan 22, 2017

Split extractCurrentResult(passthrough=True) into own function

a4aae36

dstufft force-pushed the 8900-dstufft-undefer branch from b2a120c to a4aae36 Compare January 22, 2017 19:59

exarkun mentioned this pull request Jan 23, 2017

(in-org) twisted.internet.defer.extractCurrentResult #690

Open

exarkun closed this Jan 23, 2017

		@@ -52,6 +52,12 @@ class TimeoutError(Exception):
		"""


		class InvalidStateError(Exception):

		@@ -159,6 +165,65 @@ def maybeDeferred(f, args, *kw):
		return succeed(result)


		def unbox(deferred, raises=True, passthrough=True):

		@@ -52,6 +52,12 @@ class TimeoutError(Exception):
		"""


		class InvalidStateError(BaseException):

		@@ -0,0 +1,2 @@
		Added twisted.internet.defer.unbox function to enable the unboxing of a
		"resolved" Deferred.


		@raise UnboxingWithoutResult: If C{deferred} has pending work to be done
		and has no result to unbox.

		@@ -0,0 +1,2 @@
		Added twisted.internet.defer.extractCurrentResult function to enable extracting

Add extractCurrentResult() and observeCurrentResult() to synchronously interact with a Deferred's result. #590

Add extractCurrentResult() and observeCurrentResult() to synchronously interact with a Deferred's result. #590

Conversation

dstufft commented Nov 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigc commented Nov 12, 2016

dstufft commented Nov 12, 2016

codecov-io commented Nov 13, 2016 • edited Loading

Current coverage is 90.18% (diff: 100%)

Choose a reason for hiding this comment

dstufft Nov 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Julian commented Nov 13, 2016

dstufft commented Nov 13, 2016

exarkun commented Nov 14, 2016

dstufft commented Nov 14, 2016

exarkun commented Nov 14, 2016

dstufft commented Nov 14, 2016

exarkun commented Nov 14, 2016

dstufft commented Nov 14, 2016

exarkun commented Nov 14, 2016

dstufft commented Nov 14, 2016

Julian commented Nov 14, 2016

exarkun commented Nov 14, 2016

Julian commented Nov 14, 2016

dstufft commented Nov 14, 2016

Julian commented Nov 14, 2016

Julian commented Nov 14, 2016

dstufft commented Nov 14, 2016

Choose a reason for hiding this comment

dstufft commented Dec 1, 2016

hawkowl commented Dec 1, 2016

exarkun commented Dec 1, 2016

dstufft commented Dec 1, 2016

exarkun commented Dec 1, 2016

dstufft commented Dec 1, 2016

glyph commented Dec 1, 2016

glyph commented Dec 1, 2016

termim commented Dec 1, 2016

glyph commented Dec 9, 2016

dstufft commented Dec 9, 2016

glyph commented Dec 9, 2016

exarkun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstufft commented Jan 22, 2017

exarkun commented Jan 23, 2017

dstufft commented Nov 11, 2016 •

edited

Loading

codecov-io commented Nov 13, 2016 •

edited

Loading

dstufft Nov 13, 2016 •

edited

Loading