Dispatcher cleanup #3025
Dispatcher cleanup #3025
Conversation
We use a compatibility config file to convert old plugs on loading instead.
These were going to varying degrees of trouble to ensure that they only requested tasks that had valid input connections. But these were buggy, and although we "fixed" the problem for TaskNode in a538b69, it was still wrong in several other derived classes. One of our basic rules for nodes is that they shouldn't "look outside", and should deal only with their directly owned plugs. By following that rule here, we can remove responsibility from the TaskNodes entirely, and deal with it centrally in the dispatcher.
This isn't testing anything that isn't already tested in GafferDispatch (ExecutableOpHolder doesn't reimplement `preTasks()`, so the base class tests are fine). We could update it to account for the latest changes, but I don't think it's worth it.
| @@ -648,6 +648,11 @@ class Dispatcher::Batcher | |||
| } | |||
| } | |||
|
|
|||
| const Gaffer::Plug *dispatcherPlug( const TaskNode::Task &task ) | |||
| { | |||
| return static_cast<const TaskNode *>( task.plug()->node() )->dispatcherPlug(); | |||
There was a problem hiding this comment.
I had to think a bit about whether this would ever fail. If you had a node implemented as a network, with a task output, and then you removed the internal task nodes ( maybe the network for some reason no longer needs to submit tasks ), then you could end up with source() returning an output plug on the network, which might not be a TaskNode, causing a crash? This is an obscure situation, but it means that I'm not super clear on what the rules for TaskPlugs are. You may only have an output TaskPlug on a TaskNode, or a network ( if it's connected to an internal TaskNode )?
There was a problem hiding this comment.
GafferDispatch used to be pretty hazy in this area, mixing and matching where it passed plugs vs nodes, and I'm slowly refactoring it into line with ComputeNode/ValuePlug. The general rule in all this is that nodes are the "doers" but their API is protected, and plugs provide the public API. You don't call ComputeNode::compute(), you call ValuePlug::getValue(), and you don't call TaskNode::execute(), you call TaskPlug::execute(). The rule for output TaskPlugs is also analogous to that for output ValuePlugs : the source plug needs to belong to a TaskNode. If you inadvertently build an invalid network, this will trigger the exception in TaskNodeProcess. This will be triggered before we call dispatcherPlug(), so the cast here is safe. The prior code used runTimeCast() via the removed Task::node() method, but did not check for failure, making runTimeCast() pointless. Since we have checked that we have a TaskNode already, I felt that static_cast was appropriate here.
There was a problem hiding this comment.
I figured that this was subtle enough as to warrant some test coverage, so I've pushed one further commit, which adds some. I'll merge once the CI passes.
This is a followup for #3024, since overnight I realised that some cleanup I had avoided was possible after all.