Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed issue occuring during alternating calls of getParamCached and setParam #1439

Merged
merged 10 commits into from
Apr 1, 2019

Conversation

cwecht
Copy link
Contributor

@cwecht cwecht commented Jun 20, 2018

fff8901 shows an issue which is likely a race condition in the interaction of the nodes parameter cache and the parameter server.

@cwecht cwecht closed this Jun 20, 2018
@cwecht cwecht reopened this Jun 20, 2018
@cwecht cwecht changed the title test_roscpp/params: added test for frequent alternating calls of getParamCached and setParam Fixed issue occuring during alternating calls of getParamCached and setParam Jun 21, 2018
@cwecht
Copy link
Contributor Author

cwecht commented Jun 21, 2018

To get this clear. The race, which this PR fixes, is this:

  1. The Node A writes parameter 'param' to 1 calling NodeHandle::setParam: 'param' is set in the nodes cache and is transmitted to the parameter server.
  2. A reads 'param' with NodeHandle::getParamCached and gets the value 1 out of the cache.
  3. A writes 'param' again, but this time the value 2. The value is written to the cache and transmitted to the server.
  4. The parameter server notifys all nodes, which have been using 'param' , about the parameter change from step 1. In A's cache the value of 'param' is set to 1.
  5. A read 'param' from the cache and gets value 1 (but expected 2).

The example chosen in fff8901 might be contrived, but it is a simple way to reproduce this issue.

@cwecht
Copy link
Contributor Author

cwecht commented Jun 21, 2018

Ok, as far as i can tell, the test for the rosmaster fails now, because the tests expects compute_param_updates to have a pretty wide contract. The documentation of Registrations says that its map-members is a map from keys to lists of pairs of the form (caller_id, caller_api) (L162).

But test_rosmaster_paramserver.py tests compute_param_updates passing a map from keys to caller_ids.

I find this rather confusing. Also I do not see any purpose of this rather than making the test easier.

@cwecht
Copy link
Contributor Author

cwecht commented Jun 21, 2018

Maybe there should be new tests for compute_param_updates but I'd like to get some feedback about this patch in advance.

@dirk-thomas
Copy link
Member

@ros-pull-request-builder retest this please

@cwecht
Copy link
Contributor Author

cwecht commented Jan 4, 2019

Well, two changes, which are pretty unrelated to the original purpose of this PR, fixed the builds now.
@dirk-thomas I'm willing to create separate PRs for this unrelated changes if it makes it easier for you.

@cwecht
Copy link
Contributor Author

cwecht commented Feb 20, 2019

@dirk-thomas As far as I am concerned this can be merged now.

@dirk-thomas
Copy link
Member

Thanks for the patch.

@dirk-thomas dirk-thomas merged commit 9ae132c into ros:melodic-devel Apr 1, 2019
tahsinkose pushed a commit to tahsinkose/ros_comm that referenced this pull request Apr 15, 2019
…etParam (ros#1439)

* test_roscpp/params/added getParamCachedSetParamLoop

* rosmaster: set_param: the not update the caller!

* rosmaster: set_param: do not update the caller more fine grained

* /rosmaster/paramserver/compute_params_update, apply filter only if caller_id_to_ignore is not None

* /test_rospy/talker: set publishers queue_size to supress warning

* /test_rospy/sub_to_multple_pubs: moved listener up to avoid warnings

* refactor for readability

* pep8
dirk-thomas pushed a commit that referenced this pull request Aug 4, 2020
…etParam (#1439)

* test_roscpp/params/added getParamCachedSetParamLoop

* rosmaster: set_param: the not update the caller!

* rosmaster: set_param: do not update the caller more fine grained

* /rosmaster/paramserver/compute_params_update, apply filter only if caller_id_to_ignore is not None

* /test_rospy/talker: set publishers queue_size to supress warning

* /test_rospy/sub_to_multple_pubs: moved listener up to avoid warnings

* refactor for readability

* pep8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants