Continue loading classes on error

[furushchev]

pluginlib functions stop on error on loading classes, therefore if there is only one invalid or missing plugin xml, all pluginlib functions does not work.
In this pull request, I added try-catch on loading each xml file and let this procedure continue on error with printing error message.

Currently, for example, gmapping package has missing pluginlib xml which fails nodelet working:

$ sudo apt install ros-kinetic-gmapping ros-kinetic-nodelet  # using shadow-fixed repos
$ source /opt/ros/kinetic/setup.bash
$ roscore &
$ rosrun nodelet nodelet manager
terminate called after throwing an instance of 'pluginlib::InvalidXMLException'
  what():  XML Document has no Root Element. This likely means the XML is malformed or missing.
Aborted

After the change from this pull request:

$ rosrun nodelet nodelet manager
[ERROR] [1510545753.431745765]: Skipped loading plugin with error: XML Document has no Root Element. This likely means the XML is malformed or missing. at /opt/ros/kinetic/share/gmapping/nodelet_plugins.xml.
[ INFO] [1510545753.436734671]: Initializing nodelet with 4 worker threads.

[k-okada]

+1, great! Because of this, for example rosrun image_proc image_proc crashes due to missing nodelet xml such as ros-perception/slam_gmapping#56

open("/opt/ros/kinetic/share/gmapping/nodelet_plugins.xml", O_RDONLY) = -1 ENOENT (No such file or directory)
futex(0x7f9c1d7bc680, FUTEX_WAKE_PRIVATE, 2147483647) = 0
write(2, "terminate called after throwing "..., 48terminate called after throwing an instance of ') = 48
write(2, "pluginlib::InvalidXMLException", 30pluginlib::InvalidXMLException) = 30
write(2, "'\n", 2'
)                      = 2
write(2, "  what():  ", 11  what():  )             = 11
write(2, "XML Document has no Root Element"..., 84XML Document has no Root Element. This likely means the XML is malformed or missing.) = 84
write(2, "\n", 1
)                       = 1
rt_sigprocmask(SIG_UNBLOCK, [ABRT], NULL, 8) = 0
tgkill(6592, 6592, SIGABRT)             = 0
--- SIGABRT {si_signo=SIGABRT, si_code=SI_TKILL, si_pid=6592, si_uid=1000} ---
+++ killed by SIGABRT (core dumped) +++
Aborted (core dumped)

FYI: @future731 jsk-enshu/robot-programming#223

[mikaelarguedas]

Thanks @furushchev !
I modified the PR to not add new API but use the already existing one. Let me know if there was a strong rationale behind adding API and I can revert my changes if needed

[furushchev]

@mikaelarguedas Thank you too! LGTM 👍

[mikaelarguedas]

Note to self: This change will apply only to kinetic and lunar, whether or not to backport will depend on if TinyXML behaves that same way as TinyXML2 on broken xml files

[fmessmer]

any chance this gets backported to indigo, too?

[mikaelarguedas]

@ipa-fxm sorry for the late reply. Yes I'm considering this for backport but didnt have time yet to go through various recent PRs and release a new indigo version with some of them bacported.

[mikaelarguedas]

@ipa-fxm could you please provide more context regarding why you'd like this to be backported in indigo? Do you have an example of pluginlib failing without this but passing with this patch?

It seems to me that the current behavior on kinetic (with this fix) and the one on indigo (without this fix) is the same: "Print the error message if an invalid xml file is found and keep iterating". It appears to me that the fix was needed because kinetic/lunar now use tinyxml2 that behaves differently than TinyXML. So we have to throw an exception rather than just returning (see this PR and especially this commit).

[fmessmer]

Hmm, I haven't investigated this with more detail...
Might be in our case we had another problem in one of our plugins and I hoped that it would be related to and fixed by the issue with gmapping breaking the plugin-loading mechanism...

If you investigated this with more detail, and say the behavior is the same, I think a backport is then not necessary...Thanks

I'll re-open a new issue, if our problem really turns out to be related to the plugin-loading mechanism

[mikaelarguedas]

I hoped that it would be related to and fixed by the issue with gmapping breaking the plugin-loading mechanism

Yeah I assumed the same issue would show up on all distros so that's the example I used to test the currently released indigo version. It seems to work. I'll consider providing a better error message (with the name of the culprit file for example) but the functionality should stay unchanged.

I'll re-open a new issue, if our problem really turns out to be related to the plugin-loading mechanism

Yes, if you can provide a reproducible example that's be great 👍 , I'll happily review contributions to address it