[Fixes #4826] detach ManyPromiseProxies when a new one is created

[stefanpenner]

WIP:

• switch to ManyPromiseProxies being stable

An alternative path would be to keep the proxy stable as well, and just mutate it..

[BryanCrotaz]

Am I right in thinking it's the proxy that's sent out to the outside world? If so keeping it stable would be better - I could pass it off to something else (e.g. a service) which wouldn't expect it to be destroyed later

[stefanpenner]

@BryanCrotaz yup you are correct, I am planning to take the stable proxy route.

Tou can use this code today, and it will be better then the existing. But before merging, stable proxy stuff will need to be added.

[BryanCrotaz]

I don't think you can turn today's proxy into a stable object. You'd need to replace the promise and the content.

From the docs...

promise: This property must be specified upon creation, and should not be changed once created.

What are your thoughts thus far?

[stefanpenner]

What are your thoughts thus far?

Still the same. I believe we need a stable promise proxy here.

The assertion you mentioned is in-fact what happens today, but as the person who added that assertion I feel empowered to consider changing it, or making a mutable variant. I will most likely make a mutable variant for ember-data, experiment with it, and see where it goes.

If you have time to spike a mutable variant for experimentation, that would be great...
I may have time tonight, but I am unsure.

[BryanCrotaz]

My thinking was around what happens if the original promise has not settled, but you're swapping out the promise. You can't cancel the original promise, or detach the then from it, so...

[stefanpenner]

My thinking was around what happens if the original promise has not settled, but you're swapping out the promise. You can't cancel the original promise, or detach the then from it, so...

There should be no difference in the success case, as the manyArray is already stable, and each of the promises today always fulfills with the same manyArray (the only difference is we get a fresh promise proxy). In the rejection scenario one will still get a fresh error.

Which at best seems fine, and at worst seems cost of doing business.

Thoughts?

[BryanCrotaz]

what if the old and new promise complete out of order? won't you get the wrong content?

[stefanpenner]

@BryanCrotaz I believe behavior we have today would remain the same, only improved. As for the promiseProxy thanks for pointing out the ordering concern, I'll make sure the order remains reasonable, and should likely mimic what one would experience today (or improve) but with proxy stability. Not based on the order the promises landed but the ordered the promises where associated with the proxy. (this would prevent out of order)

I suspect this effort wont just fix the memory leaks, but also improve performance. In the example you provided, I notice there was more rendering churn happening then expected. I suspect it is related to this above mentioned instability.

[BryanCrotaz]

I also want to do some more work around optimising notifications. ArrayObservers are not well served by Ember Data. The current code just clears the array and then repopulates it. I have an old branch that makes some big improvements but it's never been merged and now is so badly diverged that I can't work out how to merge it.

[BryanCrotaz]

This work could be extended by refactoring computed macro filters to use array observers - no need to check the filter on an array member if it wasn't just added.

[BryanCrotaz]

In my example we really shouldn't have any rendering at all - nothing changed in the data.

[BryanCrotaz]

@AndyPyeSC (on my team) says the promise proxy is already reusable (though we haven't tested replacing a promise that isn't yet settled), so we'll have a go at utilising that in HasMany

[AndyPyeSC]

To follow up on the comment @BryanCrotaz made above, the promise proxy mixin seems to handle very simple "linear" timelines of promise update and fulfillment as it stands. However, the case of the promise being updated before it resolves/rejects isn't handled as I'd expect it to be.

We'd also need to figure out what the state (isFulfilled, isRejected etc.) of the proxy object should be after e.g. the original promise resolves and is then swapped out for a pending one.

[BryanCrotaz]

For my money once the state isFulfilled it should stay there - you have valid content available if you ask for it.

If it isRejected then later isFulfilled then that gets interesting. Anyone listening to the external thenable will have stopped listening when the original promise settled.

I'm suggesting to Andy that we start by implementing everything other than replacing the promise while the current one is not yet settled. That will fix the memory leak and only leaves the race condition to be fixed.

[stefanpenner]

We'd also need to figure out what the state (isFulfilled, isRejected etc.) of the proxy object should be after e.g. the original promise resolves and is then swapped out for a pending one.

My initial though tis that it should update and reflect those attributes based on the current state.

[stefanpenner]

superseded by #4842