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

fix xmlrpc timeout using monotonic clock #1249

Merged
merged 8 commits into from
Feb 5, 2018

Conversation

flixr
Copy link
Contributor

@flixr flixr commented Nov 30, 2017

in XmlRpcDispatch::work the _endTime check was using system time instead of monotonic time,
so when time was set back it didn't process all events..

in XmlRpcDispatch::work the _endTime check was using system time instead of monotonic time,
so when time was set back it didn't process all events..
@mikepurvis
Copy link
Member

LGTM, but this should be confirmed to work correctly OS X and FreeBSD.

@flixr
Copy link
Contributor Author

flixr commented Nov 30, 2017

Good point.
According to https://www.unix.com/man-page/FreeBSD/2/clock_gettime/ it's available on FreeBSD, but apparently not on OSX: https://stackoverflow.com/questions/5167269/clock-gettime-alternative-in-mac-os-x

@mikepurvis
Copy link
Member

@dirk-thomas
Copy link
Member

The same code already exists in rostime. It should either be reused or at last be copied with a comment from where the code is coming and the same PP conditions should be used.

@dirk-thomas
Copy link
Member

Instead of copy-n-pasting a non-trivial code block like this wouldn't it be better to expose ros_steadytime in a header in rostime and call the function here?

@flixr
Copy link
Contributor Author

flixr commented Dec 19, 2017

I thought it would be better to not add this dependency here, but I could do that if this is preferred...

@flixr
Copy link
Contributor Author

flixr commented Dec 29, 2017

@dirk-thomas is it ok to leave it like this, or should we really add rostime as a dependency to xmlrpcpp?

@flixr
Copy link
Contributor Author

flixr commented Jan 23, 2018

ping @dirk-thomas

@dirk-thomas
Copy link
Member

I think the risk is pretty high that when the implementation in rostime is updated / fixed / modified in the future this copied code isn't. Therefore I think it is worth to expose the function in rostime in a header and update this patch to use the new function (and declare a version dependency on rostime with the next patch release.

flixr added a commit to flixr/roscpp_core that referenced this pull request Feb 2, 2018
so we can use these cross platform implementations elsewhere.
See e.g. ros/ros_comm#1249
which depends on ros/roscpp_core#73
and assumes that it will be available with rostime 0.6.9
dirk-thomas pushed a commit to ros/roscpp_core that referenced this pull request Feb 2, 2018
so we can use these cross platform implementations elsewhere.
See e.g. ros/ros_comm#1249
@dirk-thomas
Copy link
Member

dirk-thomas commented Feb 2, 2018

I will have to release roscpp_core into Lunar in order for the PR job to pass... (ros/rosdistro#16898)

@dirk-thomas
Copy link
Member

@ros-pull-request-builder retest this please


gettimeofday(&tv, &tz);
return (tv.tv_sec + tv.tv_usec / 1000000.0);
ros_steadytime(sec, nsec);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might need the ros:: namespace?

@dirk-thomas
Copy link
Member

I tried to fix this "on-the-fly" using the GitHub web interface which wasn't really successful. Can you please update the PR to make it pass CI. Thanks.

@flixr
Copy link
Contributor Author

flixr commented Feb 3, 2018

Done.
Sorry about the missing things earlier... I didn't have machine handy to test it and wrongly thought it would be trivial enough to do without tests..

@dirk-thomas
Copy link
Member

No problem. Thank you for addressing it quickly.

@dirk-thomas dirk-thomas merged commit d1536f9 into ros:lunar-devel Feb 5, 2018
@flixr flixr deleted the fix_xmlrcpp_time branch February 6, 2018 19:44
dirk-thomas pushed a commit that referenced this pull request Feb 9, 2018
* fix xmlrpc timeout using monotonic clock

in XmlRpcDispatch::work the _endTime check was using system time instead of monotonic time,
so when time was set back it didn't process all events..

* replace clock_gettime for OSX

* xmlrpcpp: copy code from ros_steadytime for getTime

* use ros_steadytime

which depends on ros/roscpp_core#73
and assumes that it will be available with rostime 0.6.9

* fold build and run depend into depend tag to avoid version to be repeated

* add missing namespace to ros_steadytime

* add missing target_link_libraries

* add missing lib to tests
kheaactua pushed a commit to kheaactua/conan-tf2 that referenced this pull request Dec 24, 2018
so we can use these cross platform implementations elsewhere.
See e.g. ros/ros_comm#1249
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants