made sure everything's thread safe
Pretty much everything already was, but I tweaked a few things to make
sure they stay that way and added various comments about that fact. Also
cleaned up a few things along the way and removed SafeMessageBuilder
because it wasn't.
diff --git a/aos/common/controls/polytope.h b/aos/common/controls/polytope.h
index 6afe04b..e37346e 100644
--- a/aos/common/controls/polytope.h
+++ b/aos/common/controls/polytope.h
@@ -19,6 +19,10 @@
k_(k) {
}
+ // This is an initialization function shared across all instantiations of this
+ // template.
+ // This must be called at least once before calling any of the methods. It is
+ // not thread-safe.
static void Init() {
dd_set_global_constants();
}
diff --git a/aos/common/die.cc b/aos/common/die.cc
index e1b3b07..69b2aef 100644
--- a/aos/common/die.cc
+++ b/aos/common/die.cc
@@ -10,6 +10,7 @@
#include <stdint.h>
#include <string>
+#include <atomic>
namespace aos {
@@ -45,7 +46,7 @@
#endif
}
-bool test_mode = false;
+::std::atomic_bool test_mode(false);
} // namespace
@@ -57,7 +58,7 @@
fputs("aos fatal: ERROR!! details following\n", stderr);
va_copy(args1, args_in);
vfprintf(stderr, format, args1);
- if (!test_mode) {
+ if (!test_mode.load()) {
fputs("aos fatal: ERROR!! see stderr for details\n", stdout);
const std::string filename = GetFilename();
@@ -78,7 +79,7 @@
}
void SetDieTestMode(bool new_test_mode) {
- test_mode = new_test_mode;
+ test_mode.store(new_test_mode);
}
} // namespace aos
diff --git a/aos/common/logging/logging_impl.cc b/aos/common/logging/logging_impl.cc
index f520b35..969c0ee 100644
--- a/aos/common/logging/logging_impl.cc
+++ b/aos/common/logging/logging_impl.cc
@@ -38,7 +38,7 @@
Context *context = Context::Get();
context->implementation = implementation;
- global_top_implementation = implementation;
+ global_top_implementation.store(implementation);
}
void NewContext() {
diff --git a/aos/common/logging/logging_impl.h b/aos/common/logging/logging_impl.h
index a55e768..f24ce6a 100644
--- a/aos/common/logging/logging_impl.h
+++ b/aos/common/logging/logging_impl.h
@@ -11,6 +11,7 @@
#include <string>
#include <functional>
+#include <atomic>
#include "aos/common/logging/logging.h"
#include "aos/common/type_traits.h"
@@ -30,7 +31,8 @@
//
// It is implemented in logging_impl.cc and logging_interface.cc. They are
// separate so that code used by logging_impl.cc can link in
-// logging_interface.cc to use logging.
+// logging_interface.cc to use logging without creating a circular dependency.
+// However, any executables with such code still need logging_impl.cc linked in.
namespace aos {
namespace logging {
@@ -250,10 +252,13 @@
// goes.
namespace internal {
-extern LogImplementation *global_top_implementation;
+extern ::std::atomic<LogImplementation *> global_top_implementation;
// An separate instance of this class is accessible from each task/thread.
// NOTE: It will get deleted in the child of a fork.
+//
+// Get() and Delete() are implemented in the platform-specific interface.cc
+// file.
struct Context {
Context();
@@ -267,6 +272,8 @@
// Deletes the Context object for this task/thread so that the next Get() is
// called it will create a new one.
// It is valid to call this when Get() has never been called.
+ // This also gets called after a fork(2) in the new process, where it should
+ // still work to clean up any state.
static void Delete();
// Which one to log to right now.
diff --git a/aos/common/logging/logging_interface.cc b/aos/common/logging/logging_interface.cc
index 8c45afd..e4c6fcd 100644
--- a/aos/common/logging/logging_interface.cc
+++ b/aos/common/logging/logging_interface.cc
@@ -15,10 +15,10 @@
namespace logging {
namespace internal {
-LogImplementation *global_top_implementation(NULL);
+::std::atomic<LogImplementation *> global_top_implementation(NULL);
Context::Context()
- : implementation(global_top_implementation),
+ : implementation(global_top_implementation.load()),
sequence(0) {
cork_data.Reset();
}
diff --git a/aos/common/macros.h b/aos/common/macros.h
index 21482f6..e8558b2 100644
--- a/aos/common/macros.h
+++ b/aos/common/macros.h
@@ -8,6 +8,7 @@
#define DISALLOW_COPY_AND_ASSIGN(TypeName) \
TypeName(const TypeName&) = delete; \
void operator=(const TypeName&) = delete
+
// A macro to wrap arguments to macros that contain commas.
// Useful for DISALLOW_COPY_AND_ASSIGNing templated types with multiple template
// arguments.
diff --git a/aos/common/mutex.h b/aos/common/mutex.h
index a22df4e..ff953e4 100644
--- a/aos/common/mutex.h
+++ b/aos/common/mutex.h
@@ -30,7 +30,7 @@
// Locks the mutex. If it fails, it calls LOG(FATAL).
void Lock();
// Unlocks the mutex. Fails like Lock.
- // Multiple unlocking might be considered failure.
+ // Multiple unlocking is undefined.
void Unlock();
// Locks the mutex unless it is already locked.
// Returns whether it succeeded or not.
@@ -55,7 +55,6 @@
// A class that locks a Mutex when constructed and unlocks it when destructed.
// Designed to be used as a local variable so that
// the mutex will be unlocked when the scope is exited.
-// Should it fail for some reason, it dies with LOG(FATAL).
class MutexLocker {
public:
explicit MutexLocker(Mutex *mutex) : mutex_(mutex) {
diff --git a/aos/common/queue.h b/aos/common/queue.h
index 33dc377..da2a180 100644
--- a/aos/common/queue.h
+++ b/aos/common/queue.h
@@ -3,17 +3,9 @@
#include <assert.h>
-#if defined(__VXWORKS__) || defined(__TEST_VXWORKS__)
-#define USE_UNSAFE
-#else
-#undef USE_UNSAFE
-#endif
-
#include "aos/common/time.h"
#include "aos/common/macros.h"
-#ifndef USE_UNSAFE
#include "aos/linux_code/ipc_lib/queue.h"
-#endif // USE_UNSAFE
#include "aos/common/time.h"
namespace aos {
@@ -53,15 +45,11 @@
template <class T> class Queue;
template <class T> class MessageBuilder;
template <class T> class ScopedMessagePtr;
-#ifndef USE_UNSAFE
-template <class T> class SafeMessageBuilder;
-template <class T> class SafeScopedMessagePtr;
-#endif // USE_UNSAFE
// A ScopedMessagePtr<> manages a queue message pointer.
// It frees it properly when the ScopedMessagePtr<> goes out of scope or gets
// sent. By design, there is no way to get the ScopedMessagePtr to release the
-// message pointer.
+// message pointer to external code.
template <class T>
class ScopedMessagePtr {
public:
@@ -116,7 +104,6 @@
reset();
}
-#ifndef SWIG
// Implements a move constructor. This only takes rvalue references
// because we want to allow someone to say
// ScopedMessagePtr<X> ptr = queue.MakeMessage();
@@ -126,14 +113,9 @@
// clear out the source so there aren't 2 pointers to the message lying
// around.
ScopedMessagePtr(ScopedMessagePtr<T> &&ptr)
- :
-#ifndef USE_UNSAFE
- queue_(ptr.queue_),
-#endif // USE_UNSAFE
- msg_(ptr.msg_) {
+ : queue_(ptr.queue_), msg_(ptr.msg_) {
ptr.msg_ = NULL;
}
-#endif // SWIG
private:
// Provide access to set_queue and the constructor for init.
@@ -141,27 +123,20 @@
// Provide access to the copy constructor for MakeWithBuilder.
friend class aos::MessageBuilder<T>;
-#ifndef USE_UNSAFE
- // Only Queue should be able to build a queue.
+ // Only Queue should be able to build a message.
ScopedMessagePtr(RawQueue *queue, T *msg)
: queue_(queue), msg_(msg) {}
-#else
- ScopedMessagePtr(T *msg)
- : msg_(msg) {}
-#endif // USE_UNSAFE
// Sets the pointer to msg, freeing the old value if it was there.
// This is private because nobody should be able to get a pointer to a message
// that needs to be scoped without using the queue.
void reset(T *msg = NULL);
-#ifndef USE_UNSAFE
// Sets the queue that owns this message.
void set_queue(RawQueue *queue) { queue_ = queue; }
// The queue that the message is a part of.
RawQueue *queue_;
-#endif // USE_UNSAFE
// The message or NULL.
T *msg_;
@@ -171,8 +146,6 @@
// Specializations for the Builders will be automatically generated in the .q.h
// header files with all of the handy builder methods.
-// This builder uses an actual shm message pointer, which is more efficient and
-// more dangerous than the linux only SafeMessageBuilder.
template <class T>
class MessageBuilder {
public:
@@ -182,21 +155,14 @@
// TODO(aschuh): Base class
// T must be a Message with the same format as the messages generated by
-// the q files.
+// the .q files.
template <class T>
class Queue {
public:
typedef T Message;
Queue(const char *queue_name)
- : queue_name_(queue_name),
-#ifdef USE_UNSAFE
- queue_msg_(&msg_)
-#else
- queue_(NULL),
- queue_msg_(NULL, NULL)
-#endif // USE_UNSAFE
- {
+ : queue_name_(queue_name), queue_(NULL), queue_msg_(NULL, NULL) {
static_assert(shm_ok<T>::value,
"The provided message type can't be put in shmem.");
}
@@ -215,6 +181,9 @@
// Returns true if there was a new message available and we successfully
// fetched it.
bool FetchNext();
+
+ // Fetches the next message from the queue, waiting if necessary until there
+ // is one.
void FetchNextBlocking();
// Fetches the last message from the queue.
@@ -227,16 +196,17 @@
void FetchAnother();
// Returns the age of the message.
- const time::Time Age() { return time::Time::Now() - queue_msg_->sent_time; }
+ const time::Time Age() const { return time::Time::Now() - queue_msg_->sent_time; }
// Returns true if the latest value in the queue is newer than age mseconds.
- bool IsNewerThanMS(int age) {
+ // DEPRECATED(brians): Use IsNewerThan(const time::Time&) instead.
+ bool IsNewerThanMS(int age) const {
// TODO(aschuh): Log very verbosely if something is _ever_ stale.
- if (get() != NULL) {
- return Age() < time::Time::InMS(age);
- } else {
- return false;
- }
+ return IsNewerThan(time::Time::InMS(age));
+ }
+
+ bool IsNewerThan(const time::Time &age) const {
+ return get() != nullptr && Age() < age;
}
// Returns a pointer to the current message.
@@ -259,40 +229,26 @@
return msg;
}
-#ifndef USE_UNSAFE
- // Returns a scoped_ptr containing a message.
- // GCC will optimize away the copy constructor, so this is safe.
- SafeScopedMessagePtr<T> SafeMakeMessage();
-
- // Returns a message builder that contains a pre-allocated message.
- aos::SafeMessageBuilder<T> SafeMakeWithBuilder();
-#endif // USE_UNSAFE
-
-#ifndef SWIG
// Returns a scoped_ptr containing a message.
// GCC will optimize away the copy constructor, so this is safe.
ScopedMessagePtr<T> MakeMessage();
// Returns a message builder that contains a pre-allocated message.
aos::MessageBuilder<T> MakeWithBuilder();
-#endif // SWIG
const char *name() const { return queue_name_; }
private:
const char *queue_name_;
-#ifdef USE_UNSAFE
- // The unsafe queue only has 1 entry and no safety, so 1 message is fine.
- T msg_;
-#else
T *MakeRawMessage();
+
// Pointer to the queue that this object fetches from.
RawQueue *queue_;
int index_ = 0;
-#endif
// Scoped pointer holding the latest message or NULL.
ScopedMessagePtr<const T> queue_msg_;
+
DISALLOW_COPY_AND_ASSIGN(Queue<T>);
};
@@ -315,11 +271,6 @@
} // namespace aos
-#ifdef USE_UNSAFE
-#include "aos/crio/queue-tmpl.h"
-#else
#include "aos/linux_code/queue-tmpl.h"
-#endif
-#undef USE_UNSAFE
#endif // AOS_COMMON_QUEUE_H_
diff --git a/aos/common/queue_test.cc b/aos/common/queue_test.cc
index dfd18a2..36522f9 100644
--- a/aos/common/queue_test.cc
+++ b/aos/common/queue_test.cc
@@ -68,30 +68,6 @@
EXPECT_EQ(0x971, my_test_queue->test_int);
}
-// Tests that we can send a message with the message pointer and get it back.
-TEST_F(QueueTest, SendMessageBlockingSafe) {
- SafeScopedMessagePtr<TestingMessage> msg = my_test_queue.SafeMakeMessage();
- msg->test_bool = true;
- msg->test_int = 0x971;
- ASSERT_TRUE(msg.SendBlocking());
-
- ASSERT_TRUE(my_test_queue.FetchLatest());
- EXPECT_TRUE(my_test_queue->test_bool);
- EXPECT_EQ(0x971, my_test_queue->test_int);
-}
-
-// Tests that we can send a message with the message pointer and get it back.
-TEST_F(QueueTest, SendMessageSafe) {
- SafeScopedMessagePtr<TestingMessage> msg = my_test_queue.SafeMakeMessage();
- msg->test_bool = true;
- msg->test_int = 0x971;
- msg.Send();
-
- ASSERT_TRUE(my_test_queue.FetchLatest());
- EXPECT_TRUE(my_test_queue->test_bool);
- EXPECT_EQ(0x971, my_test_queue->test_int);
-}
-
// Tests that we can send a message with the builder and get it back.
TEST_F(QueueTest, SendWithBuilder) {
my_test_queue.MakeWithBuilder().test_bool(true).test_int(0x971).Send();
diff --git a/aos/common/time.h b/aos/common/time.h
index 846366c..0440d91 100644
--- a/aos/common/time.h
+++ b/aos/common/time.h
@@ -205,8 +205,7 @@
}
// Enables returning the mock time value for Now instead of checking the
- // system clock. This should only be used when testing things depending on
- // time, or many things may/will break.
+ // system clock.
static void EnableMockTime(const Time &now = Now());
// Calls SetMockTime with the current actual time.
static void UpdateMockTime();