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

account for spurious wake ups from std::condition_variable::wait_for #144

Merged
merged 2 commits into from
Nov 4, 2015
Merged

account for spurious wake ups from std::condition_variable::wait_for #144

merged 2 commits into from
Nov 4, 2015

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Nov 3, 2015

On Windows this issue is most prenounced, but apparently it is possible on all platforms that std::condition_variable::wait_for can be spuriously interrupted and return std::cv_status::no_timeout even when not notified.

This logic attempts to deal with this by adding an additional bool to indicate when the system has been interrupted either by ctrl-c or rclcpp::shutdown and will recurse if the specified duration has not yet been exceeded.

@wjwwood wjwwood added the in progress Actively being worked on (Kanban column) label Nov 3, 2015
@wjwwood
Copy link
Member Author

wjwwood commented Nov 3, 2015

This bug affects the cyclic_pipeline demo.

@wjwwood wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Nov 3, 2015
@wjwwood
Copy link
Member Author

wjwwood commented Nov 3, 2015

From: http://en.cppreference.com/w/cpp/thread/condition_variable/wait_for

  1. [ ... ] It may also be unblocked spuriously. [ ... ]

@esteve
Copy link
Member

esteve commented Nov 3, 2015

+1

@jacquelinekay
Copy link
Contributor

that doesn't seem like a very useful quality of std::condition_variable.

So I assume you're discarding the timeout status of the condition variable because you no longer consider it useful (e.g. we could spuriously wake up without having timed out)?

@wjwwood
Copy link
Member Author

wjwwood commented Nov 3, 2015

Yeah, basically the return code is somewhat useless. There is a discussion about this on the Boost mailing list as well:

And the conclusion of the mailing list seems to be that if you write code which doesn't expect spurious wake ups from wait_for/wait_until then you're gonna have a bad time. Apparently this can happen on *nix systems, but is uncommon. On Windows it happens like 30% of the time for me.

@wjwwood
Copy link
Member Author

wjwwood commented Nov 3, 2015

I tested this manually on Windows, but I'll do CI for Linux and OS X:

@jacquelinekay
Copy link
Contributor

This is amazing: "In essence, if you now ask Windows to go sleep for X milliseconds,
Windows Vista onwards will in fact sleep for anywhere between zero
and X+N milliseconds where N is some arbitrarily long value."

Code looks good, waiting on CI

@jacquelinekay
Copy link
Contributor

Build failure on Linux because std::atomic is not copy constructible.

@wjwwood
Copy link
Member Author

wjwwood commented Nov 3, 2015

Beat me to it 😄.

@wjwwood
Copy link
Member Author

wjwwood commented Nov 3, 2015

New CI: http://ci.ros2.org/job/ros2_batch_ci_linux/543/

@tfoote
Copy link
Contributor

tfoote commented Nov 3, 2015

+1

jacquelinekay added a commit that referenced this pull request Nov 4, 2015
@jacquelinekay
Merge pull request #144 from ros2/fix_sleep_for_windows
26aab4f 
account for spurious wake ups from std::condition_variable::wait_for
@jacquelinekay jacquelinekay merged commit 26aab4f into master Nov 4, 2015
@jacquelinekay jacquelinekay deleted the fix_sleep_for_windows branch November 4, 2015 00:39
@jacquelinekay jacquelinekay removed the in review Waiting for review (Kanban column) label Nov 4, 2015
nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022
@wjwwood
Refactor get topic names and types (ros2#144)
6c5f98e 
* cleanup

* refactor for no_demangle and service n&t

* fixup convenience function

* add missing null check
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022
@anhosi @Karsten1987
ros2GH-144 Add missing pop for warning pragma (ros2#68)
366d750
mauropasse added a commit to mauropasse/rclcpp that referenced this pull request Aug 6, 2024
@mauropasse
Fixes for intra-process actions (ros2#144)
9177002 
* Fixes for intra-process Actions

* Fixes for Clang builds

* Fix deadlock

* Server to store results until client requests them

* Fix feedback/result data race

See ros2#2451

* Add missing mutex

* Check return value of intra_process_action_send

---------

Co-authored-by: Mauro Passerino <mpasserino@irobot.com>
mauropasse added a commit to mauropasse/rclcpp that referenced this pull request Aug 8, 2024
@mauropasse @apojomovsky
Fixes for intra-process actions (ros2#144)
69e426a 
* Fixes for intra-process Actions

* Fixes for Clang builds

* Fix deadlock

* Server to store results until client requests them

* Fix feedback/result data race

See ros2#2451

* Add missing mutex

* Check return value of intra_process_action_send

---------

Co-authored-by: Mauro Passerino <mpasserino@irobot.com>
mauropasse added a commit to mauropasse/rclcpp that referenced this pull request Aug 8, 2024
@mauropasse
Fixes for intra-process actions (ros2#144)
4c19071 
* Fixes for intra-process Actions

* Fixes for Clang builds

* Fix deadlock

* Server to store results until client requests them

* Fix feedback/result data race

See ros2#2451

* Add missing mutex

* Check return value of intra_process_action_send

---------

Co-authored-by: Mauro Passerino <mpasserino@irobot.com>
mauropasse added a commit to mauropasse/rclcpp that referenced this pull request Aug 13, 2024
@mauropasse
Fixes for intra-process actions (ros2#144)
4fa0c89 
* Fixes for intra-process Actions

* Fixes for Clang builds

* Fix deadlock

* Server to store results until client requests them

* Fix feedback/result data race

See ros2#2451

* Add missing mutex

* Check return value of intra_process_action_send

---------

Co-authored-by: Mauro Passerino <mpasserino@irobot.com>
mauropasse added a commit to mauropasse/rclcpp that referenced this pull request Aug 21, 2024
@mauropasse @jmachowinski @wjwwood
Iron: Action IPC Fixes (ros2#151)
7a51f00 
* Fixes for intra-process actions (ros2#144)

* Fixes for intra-process Actions

* Fixes for Clang builds

* Fix deadlock

* Server to store results until client requests them

* Fix feedback/result data race

See ros2#2451

* Add missing mutex

* Check return value of intra_process_action_send

---------

Co-authored-by: Mauro Passerino <mpasserino@irobot.com>

* Fix IPC Actions data race (ros2#147)

* Check if goal was sent through IPC before send responses
* Add intra_process_action_server_is_available API to intra-process Client


---------

Co-authored-by: Mauro Passerino <mpasserino@irobot.com>

* Fix data race in Actions: Part 2 (ros2#148)

* Fix data race in Actions: Part 2

* Fix warning - copy elision

---------

Co-authored-by: Mauro Passerino <mpasserino@irobot.com>

* fix: Fixed race condition in action server between is_ready and take"… (ros2#2531)

* fix: Fixed race condition in action server between is_ready and take" (ros2#2495)

Some background information: is_ready, take_data and execute data
may be called from different threads in any order. The code in the old
state expected them to be called in series, without interruption.
This lead to multiple race conditions, as the state of the pimpl objects
was altered by the three functions in a non thread safe way.

Co-authored-by: William Woodall <william@osrfoundation.org>
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

* fix: added workaround for call to double calls to take_data

This adds a workaround for a known bug in the executor in iron.

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

---------

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Co-authored-by: William Woodall <william@osrfoundation.org>

---------

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Co-authored-by: Mauro Passerino <mpasserino@irobot.com>
Co-authored-by: jmachowinski <jmachowinski@users.noreply.github.com>
Co-authored-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Co-authored-by: William Woodall <william@osrfoundation.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wjwwood @esteve @jacquelinekay @tfoote