Store a reference to the engine in receiver objects #478
Conversation
There was a problem hiding this comment.
OK, we agreed this is necessary because tasks as saved as weak references by the loop (https://docs.python.org/3/library/asyncio-task.html#asyncio.create_task). I would just add the link to the docs saying this for extra clarity and for me it is good to go.
If you create a new specific formula engine receiver that has an explicit formula engine attribute, then this wouldn't be a hack, right? You say this is a hack only because we are sticking an attribute to a receiver that doesn't have one, right?
This is a hack to ensure that the lifetime of the engine is tied to the
lifetime of the receiver. This is necessary because the engine is a task that
runs forever, and in cases where higher order built for example with the below
idiom, the user would hold no references to the engine and it could get
garbage collected before the receiver.
formula = (grid_power_engine + bat_power_engine).build().new_receiver()
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
right, so without this, the engine holds a strong reference to the task that runs the formula evaluator. But there might be cases where users don't have a reference to the engine, like in the example above. Then the user would have a reference to the receiver, and it will have a reference to the channel, but the channel doesn't hold on to anything, so the engine would be dangling, ready for clean up.
done.
I think if we do that, we'd be transferring the hack to the receiver, because it would have to have generics to get the typing right, not that we're short on typing hacks in this package. Also, it won't be enough to just create a receiver, we'd have to create a specialized channel also, because the receivers are actually created by the channels, and the |
given that this actually gives a dangling reference a place, I wouldn't call it a hack but rather a bugfix. |
It is a hack, because we are injecting an attribute with an undocumented name that can't be found anywhere in the channels repo. But as @llucax accurately put it,
But yes, it is also a bug fix. So I've updated the PR description to say just the what, and not the how. |
Yeah, true, it is not that easy to make it proper into the receiver. I didn't think it was worth writing a new receiver to make it less hacky, I think the hack is super contained and hidden, but more thinking about it there was a la hacky way to fix the bug. |
This PR fixes a bug which occurs when creating and using a higher order formula engine, without holding a reference to it.