From b82f25ec1b622bc9d671275f17dac9a7432c9925 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Wed, 7 Dec 2016 00:18:24 +0100 Subject: [PATCH 1/8] extras: improved robustness of FileDescriptorActivity * do not signal the command pipe if the trigger or break flag was already set * make sure that the IOReady step cannot be executed if the command pipe was the only active file descriptor * fixed return value of hasTimeout(): the function should return true if the current execution cycle was triggered due to a select timeout Signed-off-by: Johannes Meyer --- rtt/extras/FileDescriptorActivity.cpp | 36 +++++++++++++++++---------- rtt/extras/FileDescriptorActivity.hpp | 1 + 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/rtt/extras/FileDescriptorActivity.cpp b/rtt/extras/FileDescriptorActivity.cpp index ff890d043..f85544da8 100644 --- a/rtt/extras/FileDescriptorActivity.cpp +++ b/rtt/extras/FileDescriptorActivity.cpp @@ -82,6 +82,7 @@ FileDescriptorActivity::FileDescriptorActivity(int priority, RunnableInterface* , m_period(0) , m_has_error(false) , m_has_timeout(false) + , m_has_ioready(false) , m_break_loop(false) , m_trigger(false) , m_update_sets(false) @@ -107,6 +108,7 @@ FileDescriptorActivity::FileDescriptorActivity(int scheduler, int priority, Runn , m_period(0) , m_has_error(false) , m_has_timeout(false) + , m_has_ioready(false) , m_break_loop(false) , m_trigger(false) , m_update_sets(false) @@ -123,6 +125,7 @@ FileDescriptorActivity::FileDescriptorActivity(int scheduler, int priority, Seco , m_period(period >= 0.0 ? period : 0.0) // intended period , m_has_error(false) , m_has_timeout(false) + , m_has_ioready(false) , m_break_loop(false) , m_trigger(false) , m_update_sets(false) @@ -139,6 +142,7 @@ FileDescriptorActivity::FileDescriptorActivity(int scheduler, int priority, Seco , m_period(period >= 0.0 ? period : 0.0) // intended period , m_has_error(false) , m_has_timeout(false) + , m_has_ioready(false) , m_break_loop(false) , m_trigger(false) , m_update_sets(false) @@ -274,6 +278,7 @@ bool FileDescriptorActivity::trigger() { if (isActive() ) { { RTT::os::MutexLock lock(m_command_mutex); + if (m_trigger) return true; m_trigger = true; } int unused; (void)unused; @@ -288,7 +293,6 @@ bool FileDescriptorActivity::timeout() return false; } - struct fd_watch { int& fd; fd_watch(int& fd) : fd(fd) {} @@ -319,7 +323,7 @@ void FileDescriptorActivity::loop() } FD_SET(pipe, &m_fd_work); - int ret; + int ret = -1; m_running = false; if (m_timeout_us == 0) { @@ -327,7 +331,7 @@ void FileDescriptorActivity::loop() } else { - static const int USECS_PER_SEC = 1000000; + static const int USECS_PER_SEC = 1000000; timeval timeout = { m_timeout_us / USECS_PER_SEC, m_timeout_us % USECS_PER_SEC}; ret = select(max_fd + 1, &m_fd_work, NULL, NULL, &timeout); @@ -335,6 +339,7 @@ void FileDescriptorActivity::loop() m_has_error = false; m_has_timeout = false; + m_has_ioready = false; if (ret == -1) { log(Error) << "FileDescriptorActivity: error in select(), errno = " << errno << endlog(); @@ -345,6 +350,11 @@ void FileDescriptorActivity::loop() log(Error) << "FileDescriptorActivity: timeout in select()" << endlog(); m_has_timeout = true; } + else + { + // do not trigger an IOReady event if the only file descriptor that was active is the command pipe + m_has_ioready = !(ret == 1 && FD_ISSET(pipe, &m_fd_work)); + } // Empty all commands queued in the pipe if (ret > 0 && FD_ISSET(pipe, &m_fd_work)) // breakLoop or trigger requests @@ -369,18 +379,15 @@ void FileDescriptorActivity::loop() } // We check the flags after the command queue was emptied as we could miss commands otherwise: - bool do_trigger = true; - bool user_trigger = false; + bool do_trigger = false; { RTT::os::MutexLock lock(m_command_mutex); // This section should be really fast to not block threads calling trigger(), breakLoop() or watch(). if (m_trigger) { - do_trigger = true; - user_trigger = true; m_trigger = false; + do_trigger = true; } if (m_update_sets) { m_update_sets = false; - do_trigger = false; } if (m_break_loop) { m_break_loop = false; @@ -388,18 +395,20 @@ void FileDescriptorActivity::loop() } } - if (do_trigger) + // Execute activity... + if (m_has_timeout || m_has_ioready || do_trigger) { try { m_running = true; step(); - if (m_has_timeout) - work(RunnableInterface::TimeOut); - else if ( user_trigger ) + if ( do_trigger ) work(RunnableInterface::Trigger); - else + if ( m_has_timeout ) + work(RunnableInterface::TimeOut); + if ( m_has_ioready ) work(RunnableInterface::IOReady); + m_running = false; } catch(...) @@ -414,6 +423,7 @@ void FileDescriptorActivity::loop() bool FileDescriptorActivity::breakLoop() { { RTT::os::MutexLock lock(m_command_mutex); + if (m_break_loop) return true; m_break_loop = true; } int unused; (void)unused; diff --git a/rtt/extras/FileDescriptorActivity.hpp b/rtt/extras/FileDescriptorActivity.hpp index 1a67c92ed..6057ba539 100644 --- a/rtt/extras/FileDescriptorActivity.hpp +++ b/rtt/extras/FileDescriptorActivity.hpp @@ -115,6 +115,7 @@ namespace RTT { namespace extras { fd_set m_fd_work; bool m_has_error; bool m_has_timeout; + bool m_has_ioready; static const char CMD_ANY_COMMAND = 0; RTT::os::Mutex m_command_mutex; From 1ea78e3c6ca1fd273b9c3cf51bd5eaf263f90743 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Wed, 7 Dec 2016 00:36:12 +0100 Subject: [PATCH 2/8] extras: do not rearm the timeout timer if the previous cycle was due to a trigger() call Signed-off-by: Johannes Meyer --- rtt/extras/FileDescriptorActivity.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/rtt/extras/FileDescriptorActivity.cpp b/rtt/extras/FileDescriptorActivity.cpp index f85544da8..c3b1a82e5 100644 --- a/rtt/extras/FileDescriptorActivity.cpp +++ b/rtt/extras/FileDescriptorActivity.cpp @@ -309,6 +309,7 @@ void FileDescriptorActivity::loop() int pipe = m_interrupt_pipe[0]; fd_watch watch_pipe_0(m_interrupt_pipe[0]); fd_watch watch_pipe_1(m_interrupt_pipe[1]); + timeval timeout = { 0, 0 }; while(true) { @@ -331,9 +332,13 @@ void FileDescriptorActivity::loop() } else { - static const int USECS_PER_SEC = 1000000; - timeval timeout = { m_timeout_us / USECS_PER_SEC, - m_timeout_us % USECS_PER_SEC}; + // only rearm the timer if the previous call was not a pure command pipe event + if (m_has_timeout || m_has_ioready || m_has_error || + (timeout.tv_sec == 0 && timeout.tv_usec == 0)) { + static const int USECS_PER_SEC = 1000000; + timeout.tv_sec = m_timeout_us / USECS_PER_SEC; + timeout.tv_usec = m_timeout_us % USECS_PER_SEC; + } ret = select(max_fd + 1, &m_fd_work, NULL, NULL, &timeout); } From a2fa71dc6010063c20b62f61ddc5ca369890b3fa Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Wed, 7 Dec 2016 00:37:12 +0100 Subject: [PATCH 3/8] extras: fixed running state bookkeeping m_running was reset to false after the first call to step() and before a call to work(...). Signed-off-by: Johannes Meyer --- rtt/extras/FileDescriptorActivity.cpp | 6 ------ 1 file changed, 6 deletions(-) diff --git a/rtt/extras/FileDescriptorActivity.cpp b/rtt/extras/FileDescriptorActivity.cpp index c3b1a82e5..a34ff884d 100644 --- a/rtt/extras/FileDescriptorActivity.cpp +++ b/rtt/extras/FileDescriptorActivity.cpp @@ -325,7 +325,6 @@ void FileDescriptorActivity::loop() FD_SET(pipe, &m_fd_work); int ret = -1; - m_running = false; if (m_timeout_us == 0) { ret = select(max_fd + 1, &m_fd_work, NULL, NULL, NULL); @@ -438,18 +437,13 @@ bool FileDescriptorActivity::breakLoop() void FileDescriptorActivity::step() { - m_running = true; if (runner != 0) runner->step(); - m_running = false; } void FileDescriptorActivity::work(base::RunnableInterface::WorkReason reason) { - m_running = true; if (runner != 0) runner->work(reason); - m_running = false; - } bool FileDescriptorActivity::stop() From bedb00955c3c88738c01460783347f40e09214ca Mon Sep 17 00:00:00 2001 From: Luca Magnabosco Date: Fri, 5 May 2017 17:56:21 +0200 Subject: [PATCH 4/8] extras: removed command mutex in the FileDescriptorActivity to prevent locks in trigger() call Signed-off-by: Johannes Meyer --- rtt/extras/FileDescriptorActivity.cpp | 70 ++++++++++++--------------- rtt/extras/FileDescriptorActivity.hpp | 7 ++- 2 files changed, 34 insertions(+), 43 deletions(-) diff --git a/rtt/extras/FileDescriptorActivity.cpp b/rtt/extras/FileDescriptorActivity.cpp index a34ff884d..e4f1e9e08 100644 --- a/rtt/extras/FileDescriptorActivity.cpp +++ b/rtt/extras/FileDescriptorActivity.cpp @@ -83,10 +83,10 @@ FileDescriptorActivity::FileDescriptorActivity(int priority, RunnableInterface* , m_has_error(false) , m_has_timeout(false) , m_has_ioready(false) - , m_break_loop(false) - , m_trigger(false) - , m_update_sets(false) { + oro_atomic_set(&m_break_loop, 0); + oro_atomic_set(&m_trigger, 0); + oro_atomic_set(&m_update_sets, 0); FD_ZERO(&m_fd_set); FD_ZERO(&m_fd_work); m_interrupt_pipe[0] = m_interrupt_pipe[1] = -1; @@ -109,10 +109,10 @@ FileDescriptorActivity::FileDescriptorActivity(int scheduler, int priority, Runn , m_has_error(false) , m_has_timeout(false) , m_has_ioready(false) - , m_break_loop(false) - , m_trigger(false) - , m_update_sets(false) { + oro_atomic_set(&m_break_loop, 0); + oro_atomic_set(&m_trigger, 0); + oro_atomic_set(&m_update_sets, 0); FD_ZERO(&m_fd_set); FD_ZERO(&m_fd_work); m_interrupt_pipe[0] = m_interrupt_pipe[1] = -1; @@ -126,10 +126,10 @@ FileDescriptorActivity::FileDescriptorActivity(int scheduler, int priority, Seco , m_has_error(false) , m_has_timeout(false) , m_has_ioready(false) - , m_break_loop(false) - , m_trigger(false) - , m_update_sets(false) { + oro_atomic_set(&m_break_loop, 0); + oro_atomic_set(&m_trigger, 0); + oro_atomic_set(&m_update_sets, 0); FD_ZERO(&m_fd_set); FD_ZERO(&m_fd_work); m_interrupt_pipe[0] = m_interrupt_pipe[1] = -1; @@ -143,10 +143,10 @@ FileDescriptorActivity::FileDescriptorActivity(int scheduler, int priority, Seco , m_has_error(false) , m_has_timeout(false) , m_has_ioready(false) - , m_break_loop(false) - , m_trigger(false) - , m_update_sets(false) { + oro_atomic_set(&m_break_loop, 0); + oro_atomic_set(&m_trigger, 0); + oro_atomic_set(&m_update_sets, 0); FD_ZERO(&m_fd_set); FD_ZERO(&m_fd_work); m_interrupt_pipe[0] = m_interrupt_pipe[1] = -1; @@ -215,9 +215,7 @@ void FileDescriptorActivity::clearAllWatches() } void FileDescriptorActivity::triggerUpdateSets() { - { RTT::os::MutexLock lock(m_command_mutex); - m_update_sets = true; - } + oro_atomic_inc(&m_update_sets); int unused; (void)unused; unused = write(m_interrupt_pipe[1], &CMD_ANY_COMMAND, 1); } @@ -259,9 +257,9 @@ bool FileDescriptorActivity::start() #endif // reset flags - m_break_loop = false; - m_trigger = false; - m_update_sets = false; + oro_atomic_set(&m_break_loop, 0); + oro_atomic_set(&m_trigger, 0); + oro_atomic_set(&m_update_sets, 0); if (!Activity::start()) { @@ -277,10 +275,8 @@ bool FileDescriptorActivity::start() bool FileDescriptorActivity::trigger() { if (isActive() ) { - { RTT::os::MutexLock lock(m_command_mutex); - if (m_trigger) return true; - m_trigger = true; - } + if (oro_atomic_read(&m_trigger) > 0) return true; + oro_atomic_inc(&m_trigger); int unused; (void)unused; unused = write(m_interrupt_pipe[1], &CMD_ANY_COMMAND, 1); return true; @@ -384,19 +380,17 @@ void FileDescriptorActivity::loop() // We check the flags after the command queue was emptied as we could miss commands otherwise: bool do_trigger = false; - { RTT::os::MutexLock lock(m_command_mutex); - // This section should be really fast to not block threads calling trigger(), breakLoop() or watch(). - if (m_trigger) { - m_trigger = false; - do_trigger = true; - } - if (m_update_sets) { - m_update_sets = false; - } - if (m_break_loop) { - m_break_loop = false; - break; - } + if (oro_atomic_read(&m_trigger) > 0) { + oro_atomic_set(&m_trigger, 0); + do_trigger = true; + } + if (oro_atomic_read(&m_break_loop) > 0) { + oro_atomic_set(&m_break_loop, 0); + break; + } + if (oro_atomic_read(&m_update_sets) > 0) { + oro_atomic_set(&m_update_sets, 0); + continue; } // Execute activity... @@ -426,10 +420,8 @@ void FileDescriptorActivity::loop() bool FileDescriptorActivity::breakLoop() { - { RTT::os::MutexLock lock(m_command_mutex); - if (m_break_loop) return true; - m_break_loop = true; - } + if (oro_atomic_read(&m_break_loop) > 0) return true; + oro_atomic_inc(&m_break_loop); int unused; (void)unused; unused = write(m_interrupt_pipe[1], &CMD_ANY_COMMAND, 1); return true; diff --git a/rtt/extras/FileDescriptorActivity.hpp b/rtt/extras/FileDescriptorActivity.hpp index 6057ba539..3b4f7c18d 100644 --- a/rtt/extras/FileDescriptorActivity.hpp +++ b/rtt/extras/FileDescriptorActivity.hpp @@ -118,10 +118,9 @@ namespace RTT { namespace extras { bool m_has_ioready; static const char CMD_ANY_COMMAND = 0; - RTT::os::Mutex m_command_mutex; - bool m_break_loop; - bool m_trigger; - bool m_update_sets; + mutable oro_atomic_t m_break_loop; + mutable oro_atomic_t m_trigger; + mutable oro_atomic_t m_update_sets; /** Internal method that makes sure loop() takes into account * modifications in the set of watched FDs From 2c44a09ad8171590d8e00b77db57c1cd0d5f1788 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Mon, 15 May 2017 15:35:11 +0200 Subject: [PATCH 5/8] extras: added extra private method FileDescriptorActivity::clearCommandFlags() Signed-off-by: Johannes Meyer --- rtt/extras/FileDescriptorActivity.cpp | 30 +++++++++++++-------------- rtt/extras/FileDescriptorActivity.hpp | 5 +++++ 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/rtt/extras/FileDescriptorActivity.cpp b/rtt/extras/FileDescriptorActivity.cpp index e4f1e9e08..cf1da4fad 100644 --- a/rtt/extras/FileDescriptorActivity.cpp +++ b/rtt/extras/FileDescriptorActivity.cpp @@ -84,9 +84,7 @@ FileDescriptorActivity::FileDescriptorActivity(int priority, RunnableInterface* , m_has_timeout(false) , m_has_ioready(false) { - oro_atomic_set(&m_break_loop, 0); - oro_atomic_set(&m_trigger, 0); - oro_atomic_set(&m_update_sets, 0); + clearCommandFlags(); FD_ZERO(&m_fd_set); FD_ZERO(&m_fd_work); m_interrupt_pipe[0] = m_interrupt_pipe[1] = -1; @@ -110,9 +108,7 @@ FileDescriptorActivity::FileDescriptorActivity(int scheduler, int priority, Runn , m_has_timeout(false) , m_has_ioready(false) { - oro_atomic_set(&m_break_loop, 0); - oro_atomic_set(&m_trigger, 0); - oro_atomic_set(&m_update_sets, 0); + clearCommandFlags(); FD_ZERO(&m_fd_set); FD_ZERO(&m_fd_work); m_interrupt_pipe[0] = m_interrupt_pipe[1] = -1; @@ -127,9 +123,7 @@ FileDescriptorActivity::FileDescriptorActivity(int scheduler, int priority, Seco , m_has_timeout(false) , m_has_ioready(false) { - oro_atomic_set(&m_break_loop, 0); - oro_atomic_set(&m_trigger, 0); - oro_atomic_set(&m_update_sets, 0); + clearCommandFlags(); FD_ZERO(&m_fd_set); FD_ZERO(&m_fd_work); m_interrupt_pipe[0] = m_interrupt_pipe[1] = -1; @@ -144,9 +138,7 @@ FileDescriptorActivity::FileDescriptorActivity(int scheduler, int priority, Seco , m_has_timeout(false) , m_has_ioready(false) { - oro_atomic_set(&m_break_loop, 0); - oro_atomic_set(&m_trigger, 0); - oro_atomic_set(&m_update_sets, 0); + clearCommandFlags(); FD_ZERO(&m_fd_set); FD_ZERO(&m_fd_work); m_interrupt_pipe[0] = m_interrupt_pipe[1] = -1; @@ -219,6 +211,14 @@ void FileDescriptorActivity::triggerUpdateSets() int unused; (void)unused; unused = write(m_interrupt_pipe[1], &CMD_ANY_COMMAND, 1); } + +void FileDescriptorActivity::clearCommandFlags() +{ + oro_atomic_set(&m_break_loop, 0); + oro_atomic_set(&m_trigger, 0); + oro_atomic_set(&m_update_sets, 0); +} + bool FileDescriptorActivity::isUpdated(int fd) const { return FD_ISSET(fd, &m_fd_work); } bool FileDescriptorActivity::hasError() const @@ -256,10 +256,8 @@ bool FileDescriptorActivity::start() } #endif - // reset flags - oro_atomic_set(&m_break_loop, 0); - oro_atomic_set(&m_trigger, 0); - oro_atomic_set(&m_update_sets, 0); + // clear command flags + clearCommandFlags(); if (!Activity::start()) { diff --git a/rtt/extras/FileDescriptorActivity.hpp b/rtt/extras/FileDescriptorActivity.hpp index 3b4f7c18d..12a399a93 100644 --- a/rtt/extras/FileDescriptorActivity.hpp +++ b/rtt/extras/FileDescriptorActivity.hpp @@ -127,6 +127,11 @@ namespace RTT { namespace extras { */ void triggerUpdateSets(); + /** Internal method to clear the command (trigger, break loop, + * update sets) flags. + */ + void clearCommandFlags(); + public: /** * Create a FileDescriptorActivity with a given priority and base::RunnableInterface From 487ceef72a392191e36eea0b2eeb6d6d07aa07ae Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Fri, 27 Oct 2017 19:08:19 +0200 Subject: [PATCH 6/8] extras: ignore EINTR in FileDescriptorActivity Signed-off-by: Johannes Meyer --- rtt/extras/FileDescriptorActivity.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/rtt/extras/FileDescriptorActivity.cpp b/rtt/extras/FileDescriptorActivity.cpp index cf1da4fad..f3b78d401 100644 --- a/rtt/extras/FileDescriptorActivity.cpp +++ b/rtt/extras/FileDescriptorActivity.cpp @@ -338,8 +338,15 @@ void FileDescriptorActivity::loop() m_has_error = false; m_has_timeout = false; m_has_ioready = false; - if (ret == -1) + if (ret < 0) { + if (errno == EINTR) + { + // A signal was caught; see signal(7). We should not handle this + // here and simply continue waiting. Could be as trivial as + // a SIGWINCH (Window resize signal). + continue; + } log(Error) << "FileDescriptorActivity: error in select(), errno = " << errno << endlog(); m_has_error = true; } From 812fae0e427f80d65526ee42136431d6a95ddfa8 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Fri, 27 Oct 2017 19:13:30 +0200 Subject: [PATCH 7/8] extras: rename m_lock to m_fd_lock because its only purpose is to protect the set of watched file descriptors Signed-off-by: Johannes Meyer --- rtt/extras/FileDescriptorActivity.cpp | 10 +++++----- rtt/extras/FileDescriptorActivity.hpp | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/rtt/extras/FileDescriptorActivity.cpp b/rtt/extras/FileDescriptorActivity.cpp index f3b78d401..ba1cac715 100644 --- a/rtt/extras/FileDescriptorActivity.cpp +++ b/rtt/extras/FileDescriptorActivity.cpp @@ -182,7 +182,7 @@ void FileDescriptorActivity::setTimeout_us(int timeout_us) } } void FileDescriptorActivity::watch(int fd) -{ RTT::os::MutexLock lock(m_lock); +{ RTT::os::MutexLock lock(m_fd_lock); if (fd < 0) { log(Error) << "negative file descriptor given to FileDescriptorActivity::watch" << endlog(); @@ -194,13 +194,13 @@ void FileDescriptorActivity::watch(int fd) triggerUpdateSets(); } void FileDescriptorActivity::unwatch(int fd) -{ RTT::os::MutexLock lock(m_lock); +{ RTT::os::MutexLock lock(m_fd_lock); m_watched_fds.erase(fd); FD_CLR(fd, &m_fd_set); triggerUpdateSets(); } void FileDescriptorActivity::clearAllWatches() -{ RTT::os::MutexLock lock(m_lock); +{ RTT::os::MutexLock lock(m_fd_lock); m_watched_fds.clear(); FD_ZERO(&m_fd_set); triggerUpdateSets(); @@ -226,7 +226,7 @@ bool FileDescriptorActivity::hasError() const bool FileDescriptorActivity::hasTimeout() const { return m_has_timeout; } bool FileDescriptorActivity::isWatched(int fd) const -{ RTT::os::MutexLock lock(m_lock); +{ RTT::os::MutexLock lock(m_fd_lock); return FD_ISSET(fd, &m_fd_set); } bool FileDescriptorActivity::start() @@ -308,7 +308,7 @@ void FileDescriptorActivity::loop() while(true) { int max_fd; - { RTT::os::MutexLock lock(m_lock); + { RTT::os::MutexLock lock(m_fd_lock); if (m_watched_fds.empty()) max_fd = pipe; else diff --git a/rtt/extras/FileDescriptorActivity.hpp b/rtt/extras/FileDescriptorActivity.hpp index 12a399a93..e7752e307 100644 --- a/rtt/extras/FileDescriptorActivity.hpp +++ b/rtt/extras/FileDescriptorActivity.hpp @@ -110,7 +110,7 @@ namespace RTT { namespace extras { int m_timeout_us; //! timeout in microseconds Seconds m_period; //! intended period /** Lock that protects the access to m_fd_set and m_watched_fds */ - mutable RTT::os::Mutex m_lock; + mutable RTT::os::Mutex m_fd_lock; fd_set m_fd_set; fd_set m_fd_work; bool m_has_error; From cecf8314b8f4692b3119c1a91fa1f7b236710e4e Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Fri, 8 Dec 2017 23:07:31 +0100 Subject: [PATCH 8/8] FileDescriptorActivity: fix unit test failure related to almost concurrent removal of watched fd and write Signed-off-by: Johannes Meyer --- rtt/extras/FileDescriptorActivity.cpp | 30 +++++++++++++++++++++++---- tests/taskthread_fd_test.cpp | 3 +++ 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/rtt/extras/FileDescriptorActivity.cpp b/rtt/extras/FileDescriptorActivity.cpp index ba1cac715..2b91ed683 100644 --- a/rtt/extras/FileDescriptorActivity.cpp +++ b/rtt/extras/FileDescriptorActivity.cpp @@ -389,14 +389,36 @@ void FileDescriptorActivity::loop() oro_atomic_set(&m_trigger, 0); do_trigger = true; } + if (oro_atomic_read(&m_update_sets) > 0) { + oro_atomic_set(&m_update_sets, 0); + + // Check if file descriptors that have work also have been removed in the current cycle and + // already ignore them. This is a corner case and still triggering an IOReady event would + // probably be fine in real-world applications, but it can break the task-test unit test + // depending on timing. + // + // The simple solution would be to continue without triggering the component, but we might miss + // triggers or activities on other file descriptors then. + // + // Disabled for now and patched in the unit test. + // +// { RTT::os::MutexLock lock(m_fd_lock); +// fd_set copy; +// FD_ZERO(©); +// m_has_ioready = false; +// for(int i = 0; i <= max_fd; ++i) { +// if (FD_ISSET(i, &m_fd_set) && FD_ISSET(i, &m_fd_work)) { +// FD_SET(i, ©); +// m_has_ioready = true; +// } +// } +// m_fd_work = copy; +// } + } if (oro_atomic_read(&m_break_loop) > 0) { oro_atomic_set(&m_break_loop, 0); break; } - if (oro_atomic_read(&m_update_sets) > 0) { - oro_atomic_set(&m_update_sets, 0); - continue; - } // Execute activity... if (m_has_timeout || m_has_ioready || do_trigger) diff --git a/tests/taskthread_fd_test.cpp b/tests/taskthread_fd_test.cpp index 73c414dec..79f91b5a8 100644 --- a/tests/taskthread_fd_test.cpp +++ b/tests/taskthread_fd_test.cpp @@ -218,6 +218,9 @@ BOOST_AUTO_TEST_CASE(testFileDescriptor_Write ) mtask->unwatch(mcomp.fd[0]); BOOST_CHECK( mtask->isWatched(mcomp.fd[0]) == false ); + // sleep to give the FileDescriptorActivity some time to update the internal file descriptor set + usleep(100000/10); + ++ch; rc = write(mcomp.fd[1], &ch, sizeof(ch)); if (1 != rc) std::cerr << "rc=" << rc << " errno=" << errno << ":" << strerror(errno) << std::endl;