#621 Termination clear out state after term detected #622
Conversation
lifflander
requested review from
PhilMiller,
pnstickne,
nlslatt and
uhetmaniuk
| // We are likely inside code that references the map structure that holds DS | ||
| // terminators. We can remove it safely. So we will enqueue an action to do | ||
| // the cleanup. This action must see if the DS term is still disengaged (it | ||
| // can easily be re-engaged after it disengages) |
There was a problem hiding this comment.
This comment on outstanding references doesn't give me a whole lot of confidence that we won't have dangling reference bugs anyway.
If we want to be safe against that, we can work with objects whose lifetimes actually track such references to specific epochs, rather than the raw epoch numbers.
Alternately, the interface presented by StateDS can be better encapsulated so that references don't leak out, and it handles operations on epochs by number regardless of what state they've ended up in.
There was a problem hiding this comment.
I've improved the comment in the code. I agree with you about lifetimes, but that would be a major refactor. Are you comfortable with the code as is?
There was a problem hiding this comment.
If it holds up in practice for now, yeah, I'm fine going ahead with this.
|
I think your fixes are OK, but I'll have to spend a bunch more time with the surrounding code to be more confident. |
lifflander
force-pushed
the
621-epoch-leak
branch
from
d45f24e to
049fffb
Compare
Fixes #621