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.

Sign up for GitHub

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

Jump to bottom

Avoid timer period being set to 0 #359

Merged
merged 1 commit into from
Dec 18, 2018
Merged

Avoid timer period being set to 0 #359

merged 1 commit into from
Dec 18, 2018

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Dec 14, 2018 •
edited
Loading

Fixes ros2/examples#221

_recalculate_expire_timer() checks the time left on each of the goal handles and is supposed to set the timer period to the smallest one. The bug is it currently starts the minimum_period at 0, so the timer period is always set to 0 causing 100% CPU usage.

This starts the minimum period at the goal expiration timeout since that's the maximum time the server might have to wait to expire a goal.

connects to ros2/examples#221

@sloretz sloretz added the bug Something isn't working label Dec 14, 2018
@sloretz sloretz self-assigned this Dec 14, 2018
@sloretz sloretz added the in progress Actively being worked on (Kanban column) label Dec 14, 2018
@sloretz
Copy link
Contributor Author

sloretz commented Dec 14, 2018

The concerning thing with this patch is I don't know why the server wasn't always using 100% CPU as soon as a goal canceled. It's possible there is another bug elsewhere that's not starting the expire timer.

@sloretz sloretz added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Dec 14, 2018
wjwwood
wjwwood approved these changes Dec 18, 2018
View reviewed changes
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm, but

The concerning thing with this patch is I don't know why the server wasn't always using 100% CPU as soon as a goal canceled. It's possible there is another bug elsewhere that's not starting the expire timer.

Also concerns me...

@sloretz
Copy link
Contributor Author

sloretz commented Dec 18, 2018 •
edited
Loading

CI (testing packages above rcl_action)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@sloretz sloretz merged commit 5162a99 into master Dec 18, 2018
@sloretz sloretz deleted the sloretz/action-server-100-percent-cpu branch December 18, 2018 19:39
@sloretz sloretz removed the in review Waiting for review (Kanban column) label Dec 18, 2018
@clalancette clalancette mentioned this pull request Jan 7, 2019
Closed
emersonknapp pushed a commit to emersonknapp/rcl that referenced this pull request Feb 13, 2019
@xabxx
make pr ready for ros2cli security feature (ros2#1)
b1735ba 
* update docs about possibility of rcl_take no taking (ros2#356)

* update rcl_wait doc with respect to subs and possibility of failing takes

* add a note about possible failing takes in rcl_take docs

* 0.6.2

* Set rmw_wait timeout using ros timers too (ros2#357)

* 0.6.3

* Avoid timer period being set to 0 (ros2#359)

* Fix logic that moves goal handles when one expires (ros2#360)

* Fix error from uncrustify v0.68 (ros2#364)

* Ensure that context instance id storage is aligned correctly (ros2#365)

* Ensure that context instance id storage is aligned correctly

* Make alignment compatible with MSVC

* Namespace alignment macro with RCL_

* [rcl] Guard against bad allocation calling rcl_arguments_copy() (ros2#367)

* [rcl] Add test for copying arguments struct with no arguments

* Override allocate function in test to reveal bug

* [rcl] Only allocate arrays if there are things to copy in rcl_argument_copy()

Also guard against freeing invalid pointers if rcl_argument_copy() fails.

* Remove uncessary guard against NULL pointer

* linter, styles, uncrustify fixes
emersonknapp pushed a commit to emersonknapp/rcl that referenced this pull request Feb 13, 2019
@xabxx
make pr ready for ros2cli security feature (ros2#1)
628a61c 
* update docs about possibility of rcl_take no taking (ros2#356)

* update rcl_wait doc with respect to subs and possibility of failing takes

* add a note about possible failing takes in rcl_take docs

* 0.6.2

* Set rmw_wait timeout using ros timers too (ros2#357)

* 0.6.3

* Avoid timer period being set to 0 (ros2#359)

* Fix logic that moves goal handles when one expires (ros2#360)

* Fix error from uncrustify v0.68 (ros2#364)

* Ensure that context instance id storage is aligned correctly (ros2#365)

* Ensure that context instance id storage is aligned correctly

* Make alignment compatible with MSVC

* Namespace alignment macro with RCL_

* [rcl] Guard against bad allocation calling rcl_arguments_copy() (ros2#367)

* [rcl] Add test for copying arguments struct with no arguments

* Override allocate function in test to reveal bug

* [rcl] Only allocate arrays if there are things to copy in rcl_argument_copy()

Also guard against freeing invalid pointers if rcl_argument_copy() fails.

* Remove uncessary guard against NULL pointer

* linter, styles, uncrustify fixes
jacobperron pushed a commit that referenced this pull request Feb 21, 2019
@AAlon @jacobperron
Security directory lookup improvements (#332)
b0a124d 
* Changing security directory lookup to a prefix match rather than exact match.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Changing security directory lookup to a prefix match rather than exact match.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Changing security directory lookup to a prefix match rather than exact match.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Changing security directory lookup to a prefix match rather than exact match.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Adding security_directory module and moving rcl_get_secure_root function to it. Adding tests.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Adding security_directory module and moving rcl_get_secure_root function to it. Adding tests.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Adding security_directory module and moving rcl_get_secure_root function to it. Adding tests.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Adding security_directory module and moving rcl_get_secure_root function to it. Adding tests.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Adding security_directory module and moving rcl_get_secure_root function to it. Adding tests.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Changing security directory prefix matching to be optional. Improving error messages around security directory lookup.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Fixing get_best_matching_directory so that it always fetches the next file inside the loop.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* make pr ready for ros2cli security feature (#1)

* update docs about possibility of rcl_take no taking (#356)

* update rcl_wait doc with respect to subs and possibility of failing takes

* add a note about possible failing takes in rcl_take docs

* 0.6.2

* Set rmw_wait timeout using ros timers too (#357)

* 0.6.3

* Avoid timer period being set to 0 (#359)

* Fix logic that moves goal handles when one expires (#360)

* Fix error from uncrustify v0.68 (#364)

* Ensure that context instance id storage is aligned correctly (#365)

* Ensure that context instance id storage is aligned correctly

* Make alignment compatible with MSVC

* Namespace alignment macro with RCL_

* [rcl] Guard against bad allocation calling rcl_arguments_copy() (#367)

* [rcl] Add test for copying arguments struct with no arguments

* Override allocate function in test to reveal bug

* [rcl] Only allocate arrays if there are things to copy in rcl_argument_copy()

Also guard against freeing invalid pointers if rcl_argument_copy() fails.

* Remove uncessary guard against NULL pointer

* linter, styles, uncrustify fixes

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Update rcl/include/rcl/security_directory.h

Co-Authored-By: AAlon <avishayalon@gmail.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Adding line break in docstring

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Removing duplicate doc string

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Removing tinydir from the source tree, instead using the ROS package tinydir_vendor.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Removing tinydir

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Reformatting license notice as per linter template.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Update test_security_directory.cpp

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Changing input to putenv to be a global, statically allocated buffer.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* test_security_directory - Using a larger buffer for env string manipulations.

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Copy environment variable to allocated string so it is not clobbered by next lookup

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Address review comments

fix security directory exact match comment and unset env vars before tests

Signed-off-by: Emerson Knapp <eknapp@amazon.com>

* Remove strncpy

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjwwood wjwwood wjwwood approved these changes

@jacobperron jacobperron jacobperron approved these changes

Assignees

@sloretz sloretz

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sloretz @wjwwood @jacobperron