add .unwrap_and_destroy() method to remove reference to value/error when unwrapping outcome

[graingert]

Fixes #47

currently unwraping exceptions leaves a refcycle that keeps the entire traceback and any locals alive. This PR resolves that by clearing the error field when the Error is unwrap_and_destroy-ed.

For the previous behaviour of Value (eg in trio) if you receive a large bytes object in a Value that bytes object stays alive long after it's needed. This PR resolves that by clearing the value field when the Value is unwrap_and_destroy-ed.

this is an alternative approach to #45 that is fully backwards compatible and will only need a minor version bump, eg outcome==1.4.0

[CoolCat467]

As far as I can tell this looks like a good approach.

[A5rocks]

I like the idea of at some point making .unwrap() do this too, because this seems unintuitive. I would be happy with either this or the prior PR (disregarding comments), as long as we plan to make the breaking change at some point.

I'm not sure it helps to provide a .unwrap_and_destroy() if we're going to end up breaking things by updating .unwrap()? The only harm is that we gain an extra method, but I'm not sure if there's benefits to this.

(or does this footgun not really matter in most cases? then maybe breaking everything isn't a good idea... sorry, I don't really know the context for this change)

[graingert]

@A5rocks here's the benefit of unwrap_and_destroy: https://github.com/python-trio/outcome/pull/49/changes#diff-57edcfc849c5b8867dc8c235f66164cfa33cab5f855acce0d9defa4f23bce499R115

[graingert]

as long as we plan to make the breaking change at some point

when I make the change in trio to use the new method I'll change the dep on outcome to outcome ~=1.4

[A5rocks]

Hmm, I admit I still don't really get it. Like, here's the way I see it (which I guess must contain something wrong)... my core assumption is that this is a footgun we'd like to remove:

1. merge this PR, release a minor version bump, update unwrap, major version bump, remove this unwrap_and_destroy, major version bump (or maybe as part of 2.0).
2. merge remove reference to value/error when unwrapping outcome, and add .peek() method #45, release a major version.

Now there's a reason to do 1: it would allow us to change Trio without breaking past versions -- until we do the major version bump. However, given we do the major version anyways, anyone on an existing version of Trio is technically using a version of outcome w/ breaking changes.

It may be valuable to have the extra few versions on Trio with a unwrap_and_destroy... except it sounds like we don't need that, and that current Trio is compatible with the breaking version of unwrap? So basically, I don't see any extra use to Trio for the extra effort 1 requires over 2. Maybe there's other affected apps we care about? (though, either way we're doing the major change so those affected apps will be affected anyways)

[graingert]

Personally I think this is a work-around for a bug in CPython (and pypy) in genobject.c's send/throw method and I'm leaving test_unwrap_leaves_a_refcycle as a canary test to see if this is ever fixed in python implementations

So I don't think it's so bad to have two different unwrap methods

[A5rocks]

Sorry for the delay. I hadn't really processed your test case before, that's very interesting! I've been poking at it, but nothing to report yet (it just seems that the outcome is kept alive for some reason, who knows why!). I'll try poking at it some more and see if I can find anything!

I agree that the proper fix for that outcome is to kill the outcome (i.e. by making whatever is holding it... not...), rather than break the link between it and the exception. Not sure how I feel about a patch in outcome as a work-around (that users have to apply, in addition!). But it can't hurt, I suppose.

I was under the assumption we also wanted these changes because maybe the traceback would point to the outcome! But I can't actually find examples of that in Trio, so I guess that doesn't matter?

[A5rocks]

I was under the wrong impression before!

[A5rocks]

Hmm, I was convinced by the Claude output you shared on Gitter that this was an issue because CPython isn't stealing a reference, but... I don't get it, because I can't see where the INCREF/DECREF macros are for gen_send? But it's definitely happening, cause this:

-snip-

---

edit: nvm it was just in another file through PyStackRef_FromPyObjectNew. Bit strange, because I don't see why this needs to be?

edit2: ok, this just happens for all Python->C->Python things, I think. Maybe other stuff too! But I reproduced using operator.call. That seems pretty general, so now I'm more of a +1 on this!

[A5rocks]

Hmm, actually, given this doesn't actually solve a refcycle... I do think this CPython behavior is Bad, and I guess this can keep large values alive longer than necessary (until the next await, rather than until they get discarded.)

I'm a bit conflicted now, sorry! FWIW I'd be happy to make an outcome release whenever, so this isn't time bound. (and IMO updating Trio's requirement isn't necessary)

What do you think?

[A5rocks]

Actually, this isn't a refcycle I think.

(a conversation on Python Discord convinced me this is because generator.send holds a ref, which means that until it ends the outcome will stay alive. no refcycles here!)