Idea #8543
closed
[NodeManager] Don't use Futures when not expecting a reply
Description
Quoting #8437
on_failure is only called when there is no future associated with the message. As it turns out, all calls that use ActorProxy have an associated Future object, and all messaging between actors in node manager uses ActorProxy. This means unhandled exceptions are stored in a Future object to be returned to the caller. However, if the caller never calls get() on the Future object (because it never stored it), this means the exception is silently ignored.
These lingering future objects may also be creating circular references that is causing the memory leak.
Anywhere a message is sent between Actors where a response is not required, such as subscriptions, use tell() on the ActorRef object instead of using ActorProxy (which uses ask()). (May want to write a helper similar to ActorProxy, such as TellActorProxy).
Anticipated benefits:
- Unhandled exceptions will be logged, and the on_failure method of the Actor be called as intended
- Possibly uncover previously unknown bugs (previously hidden due to exceptions being lost)
- May mitigate longstanding memory leak bug by avoiding creating Future objects that are not needed (and which may be actively harmful)
Related issues
Updated by Peter Amstutz about 8 years ago
- Description updated (diff)
- Category set to Node Manager
Updated by Brett Smith about 8 years ago
- Target version set to Arvados Future Sprints
Updated by Brett Smith about 8 years ago
- Target version changed from Arvados Future Sprints to 2016-03-16 sprint
Updated by Tom Clegg about 8 years ago
Looking at 8543-nodemanager-fewer-futures @ bf49d05...
Could you squash this branch and synthesize all the commit messages? I'd like to have less noise in the git history, like commit logs that just say "tests pass", and these changes that got made and then reverted...- self.subscribers = None + self.subscribers.clear()
del self.shutdowns[key] + if key in self.sizes_booting_shutdown: del self.sizes_booting_shutdown[key]
This error message doesn't scan. I assume the "has no attribute" part is a copy/paste error?
raise AttributeError('%s not a callable on %s has no attribute' % (name, self))
BaseNodeMangerActor should probably be BaseNodeManagerActor
I think it would be worth providing a little more documentation beyond "Like ActorProxy, except only permits asynchronous stuff". Even coming fresh from this story description I find the workings of the "Tell" stuff a bit mysterious. Perhaps it would help to stick a docstring on tell_proxy() saying when one should use that instead of proxy()?
exception_type and/or exception_value here...
lg.critical("Unhandled exception is a fatal error, killing Node Manager")
Updated by Peter Amstutz about 8 years ago
Tom Clegg wrote:
Looking at 8543-nodemanager-fewer-futures @ bf49d05...
Could you squash this branch and synthesize all the commit messages? I'd like to have less noise in the git history, like commit logs that just say "tests pass", and these changes that got made and then reverted...
- [...]
- [...]
Rebased and squashed.
This error message doesn't scan. I assume the "has no attribute" part is a copy/paste error?
[...]
Yes I think so.
BaseNodeMangerActor should probably be BaseNodeManagerActor
Thanks for catching that.
I think it would be worth providing a little more documentation beyond "Like ActorProxy, except only permits asynchronous stuff". Even coming fresh from this story description I find the workings of the "Tell" stuff a bit mysterious. Perhaps it would help to stick a docstring on
tell_proxy()saying when one should use that instead ofproxy()?
I recall now being interrupted in the middle of starting to write that comment and obviously I forgot to come back to it. Expanded docstrings for those clasess.
This is a complaint about existing code, not the new code, but I can't help thinking it should be easy/worthwhile to logexception_typeand/orexception_valuehere...
- [...]
The exception already gets logged by pykka. It doesn't show up in the tests because the exception gets logged as "ERROR" but the tests by default only log "CRITICAL". Try ANMTEST_LOGLEVEL=ERROR python setup.py test --test-suite=tests.test_failure to see for yourself. In production we currently run at log level INFO or DEBUG.
Updated by Peter Amstutz about 8 years ago
- Status changed from New to Resolved
Applied in changeset arvados|commit:2dbbaaefc6a4a46a7f17b9e7799fc455cd722113.