Protect pop() and pop_back() operations with a mutex

[haudren]

This avoids popping twice the same element, that results in a double free and crash, see #837.

This PR should be retrocompatible with 8.04 and older releases, as long as they support boost::lock_guard (In boost_thread). It especially avoids depending on newer C++0x or C++11 constructs.

This adds a link dependency on boost_system on many components, because AutoBalancer and EmergencyStopper directly include interpolator.cpp. I think this is a bad practice and should be avoided, but I wanted to keep the impact of this PR minimal.

@k-okada can you confirm that this builds on your side ? I am not sure of which version of Boost you are using, and in which version lock_guard was introduced.

I think that the lock is mostly uncontested so this should not cause any performance issues, but tell me if you run into problems.

[k-okada]

Refer to this link for build results (access rights to CI server needed):
http://jenkins.jsk.imi.i.u-tokyo.ac.jp:8080/job/hrpsys-qnx/2381/

Build Log
last 10 lines

[...truncated 7680 lines...]
    at org.jenkinsci.plugins.ghprb.GhprbRepository.createCommitStatus(GhprbRepository.java:122)
    at org.jenkinsci.plugins.ghprb.GhprbBuilds.onCompleted(GhprbBuilds.java:118)
    at org.jenkinsci.plugins.ghprb.GhprbBuildListener.onCompleted(GhprbBuildListener.java:27)
    at org.jenkinsci.plugins.ghprb.GhprbBuildListener.onCompleted(GhprbBuildListener.java:12)
    at hudson.model.listeners.RunListener.fireCompleted(RunListener.java:201)
    at hudson.model.Run.execute(Run.java:1786)
    at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
    at hudson.model.ResourceController.execute(ResourceController.java:98)
    at hudson.model.Executor.run(Executor.java:408)

Test FAILed.

[haudren]

I modified a bit the code so that it should be retro-compatible for Ubuntu 12.04 with boost 1.48 (The location of lock_guard() was changed between 1.48 and 1.54).

@k-okada : can you confirm which version of boost you are using ? I checked and it seems that the default version on 8.04 was 1.34 that does not implement lock_guard. In that case, we will have to put some preprocessor guards and use scoped_lock instead (It has been deprecated since and is no longer present in 1.54...)

[k-okada]

Refer to this link for build results (access rights to CI server needed):
http://jenkins.jsk.imi.i.u-tokyo.ac.jp:8080/job/hrpsys-qnx/2382/
Test PASSed.

[snozawa]

Dear haudren.

Sorry for late reply.
I checked boost version on our HRP2 robot.
As you said, the version was 1.34.

In that case, we will have to put some preprocessor guards and use scoped_lock instead (It has been deprecated since and is no longer present in 1.54...)

In order to processor guards, what do you think about using this?
https://github.com/fkanehiro/hrpsys-base/blob/master/rtc/SequencePlayer/SequencePlayer.cpp#L358
SequencePlayer already uses this.
This comes from OpenRTM library and it is reasonable for hrpsys-base to depend on OpenRTM.

Moreover, why are you adding guards to pop() and pop_back() instead of adding guards to seqplay.* and SequencePlyaer.*?
Already some guards are included in SequencePlayer like this:
https://github.com/fkanehiro/hrpsys-base/blob/master/rtc/SequencePlayer/SequencePlayer.cpp#L358
, so it seems to be better to add guard to SequencePlayer.

[eisoku9618]

FYI: A table in this comment shows a boost version of each JSK robot. #751 (comment)

[haudren]

@snozawa
Thank you for suggesting to use coil::guard, it seems that it is a better idea than re-implementing our own.

For your other remark, the problem comes when someone pops_back() and pops() at the same time the last element. I wanted to protect the stack at the closest of the problem, in order to also avoid crashes in other components that may rely on interpolator.
Also, if we protect directly in SequencePlayer.cpp, we have to basically block while clear() is going on (this may take a long time if we are clearing a long pattern). I do not know if this the desired behaviour.
We can block in seqplay.cpp, but this does not change anything really, except that other components who directly rely on interpolator.cpp may still crash.

For me the question is the following: while clearing, should the SequencePlayer continue playing the current pattern ? Do we want to protect the stack against double-free in the SequencePlayer only or every component ?

[haudren]

As I commented in the issue, this is also present in the robot, and thus may crash the robot whenever clearing the SequencePlayer (Or any interpolator that is also being popped at the same time).

@fkanehiro : Any opinion on how we can fix this problem ? I think using coil::Guard is the best option, but where to put them ?

[haudren]

Hello,

I just updated my branch to use coil::Guardcoil::Mutex. If anyone would prefer me to move the guards higher up (In seqplay for example) please say so.

@fkanehiro : Any opinion on this matter ?
Cheers,
Hervé

[k-okada]

Refer to this link for build results (access rights to CI server needed):
http://jenkins.jsk.imi.i.u-tokyo.ac.jp:8080/job/hrpsys-qnx/2388/
Test PASSed.

[k-okada]

sorry for late response, could you send PR for version which uses coil?

Thanks in advance.

◉ Kei Okada

On Fri, Oct 16, 2015 at 10:45 AM, Hervé Audren notifications@github.com
wrote:

Hello,

I just updated my branch to use coil::Guardcoil::Mutex. If anyone would
prefer me to move the guards higher up (In seqplay for example) please say
so.

@fkanehiro https://github.com/fkanehiro : Any opinion on this matter ?
Cheers,
Hervé

—
Reply to this email directly or view it on GitHub
#839 (comment)
.

[haudren]

@k-okada : The current version of this PR uses coil instead of Boost. The last commit (c5a51da) removes Boost guards and adds coil Guards. If this satisfies everyone, I will squash the commits together before final merge.

Cheers,
Hervé

[k-okada]

ok, checking them on http://jenkins.jsk.imi.i.u-tokyo.ac.jp:8080/job/hrpsys-qnx/2389/console and https://travis-ci.org/fkanehiro/hrpsys-base/builds/85652312

[haudren]

Ok so the test passed on QNX, but the build errored on Travis due to:

g++: internal compiler error: Killed (program cc1plus)

I guess the virtual machine ran out of memory or something similar...

[k-okada]

Travis also passed, If this is ok, please merge @fkanehiro

[snozawa]

Sorry for late reply, too, and LGTM.