Throwing RestartNeeded from onUpdateParameters() is broken at task creation time

[rmsc]

Manager::createTask() calls Task::loadConfig(), which then calls updateParameters(). If the task's onUpdateParameters() throws a RestartNeeded, it will go all the way up to Manager::createTask().

Manager::createTask() has two exception catchers, one for std::runtime_error and a "catch-all" which just prints a simple "unknown exception" error message. RestartNeeded is NOT derived from std::runtime_error.

It is normal practice to throw a RestartNeeded whenever onUpdateParameters() fails. If it fails at task creation, the task doesn't get restarted and there's no informative error message as to why this happened. This may happen if the user forgets to set one of the mandatory task parameters, for instance, or sets a parameter to an illegal value.

A quick fix would be just make RestartNeeded derive from std::runtime_error, which at least prints a useful error message. Unfortunately, this also means that the task is not restarted, and after correctly setting the parameters the user has to restart DUNE.

A better fix would be to explicitly catch RestartNeeded exceptions when calling Task::loadConfig(), and attempt to restart the task at a later time. This may however involve some complex changes to the task creation logic.

[rmsc]

Sorry for the insistence, but this report is almost a month old.

I've proposed a fix here on pull-request #81, did any of you guys have some time to take a look?

[krisklau]

I agree that a first fix would be to at least print an error message, as simply restarting the task likely wont do anything, as the same parameters are read from the same file.

On the other hand, I guess it could be argued that an invalid setting, resulting in a task not being able to start in a known state, should result in DUNE not starting at all.

An alternative would be to start the task with default parameters, but that could also result in some unexpected behaviour, from the users perspective.

I like the fix you proposed in #81.

[josebraga]

@mariacosta please rule this. I'd say to decide or just open a poll to the community. Otherwise this will be here in perpetuaty.

[mariacosta]

Merged pull request #81.