Skip to content

Commit

Permalink
Remove global logging mutex
Browse files Browse the repository at this point in the history
Reverts most of #562

ros2/rclcpp#1042 describes a crash that can occur in rclcpp when rcl
logging functions are called in different threads. This was fixed in
ros2/rclcpp#1125, and a similar fix was made for rclpy in #562.

This fix is unnecessary in rclpy because it cannot call the logging
macros from multiple threads unless the GIL is released. The only place
the GIL is released is around rcl_wait(), so the logging methods are
already protected.

Removing this code makes it a little easier to divide the remaining work
of porting _rclpy.c to pybind11. If for some reason we decide to release
the GIL around logging methods in the future, then they can be protected
in the future using `pybind11::call_guard<T>` with a type that locks a
global logging mutex when it is default constructed and unlocks it when
its destructed.

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
  • Loading branch information
sloretz committed Mar 11, 2021
1 parent 49f2b1f commit 7433795
Show file tree
Hide file tree
Showing 8 changed files with 6 additions and 327 deletions.
3 changes: 0 additions & 3 deletions rclpy/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,6 @@ install(TARGETS rclpy_common
# Main extension with bulk of the code
pybind11_add_module(_rclpy SHARED
src/rclpy/_rclpy.c
src/rclpy/detail/execute_with_logging_mutex.cpp
src/rclpy/detail/logging_mutex.cpp
src/rclpy/detail/thread_safe_logging_output_handler.cpp
)
target_link_libraries(_rclpy PRIVATE
rclpy_common
Expand Down
44 changes: 6 additions & 38 deletions rclpy/src/rclpy/_rclpy.c
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,6 @@
#include <rmw/validate_node_name.h>
#include <rosidl_runtime_c/message_type_support_struct.h>

#include "./detail/thread_safe_logging_output_handler.h"
#include "./detail/execute_with_logging_mutex.h"
#include "rclpy_common/common.h"
#include "rclpy_common/handle.h"

Expand Down Expand Up @@ -529,7 +527,7 @@ rclpy_init(PyObject * module, PyObject * args)
* Raises RuntimeError if rcl logging could not be initialized
*/
static PyObject *
rclpy_logging_configure_impl(PyObject * module, PyObject * args)
rclpy_logging_configure(PyObject * module, PyObject * args)
{
rclpy_module_state_t * module_state = (rclpy_module_state_t *)PyModule_GetState(module);
if (!module_state) {
Expand All @@ -547,10 +545,8 @@ rclpy_logging_configure_impl(PyObject * module, PyObject * args)
return NULL;
}
rcl_allocator_t allocator = rcl_get_default_allocator();
rcl_ret_t ret = rcl_logging_configure_with_output_handler(
&context->global_arguments,
&allocator,
rclpy_detail_thread_safe_logging_output_handler);
rcl_ret_t ret = rcl_logging_configure(
&context->global_arguments, &allocator);
if (RCL_RET_OK != ret) {
PyErr_Format(
module_state->RCLError,
Expand All @@ -561,19 +557,12 @@ rclpy_logging_configure_impl(PyObject * module, PyObject * args)
Py_RETURN_NONE;
}

/// See rclpy_logging_configure_impl above.
static PyObject *
rclpy_logging_configure(PyObject * self, PyObject * args)
{
return rclpy_detail_execute_with_logging_mutex(rclpy_logging_configure_impl, self, args);
}

/// Finalize rcl logging
/**
* Produces a RuntimeWarning if rcl logging could not be finalized
*/
static PyObject *
rclpy_logging_fini_impl(PyObject * Py_UNUSED(self), PyObject * Py_UNUSED(args))
rclpy_logging_fini(PyObject * Py_UNUSED(self), PyObject * Py_UNUSED(args))
{
rcl_ret_t ret = rcl_logging_fini();
if (RCL_RET_OK != ret) {
Expand All @@ -589,16 +578,9 @@ rclpy_logging_fini_impl(PyObject * Py_UNUSED(self), PyObject * Py_UNUSED(args))
Py_RETURN_NONE;
}

/// See rclpy_logging_fini_impl above.
static PyObject *
rclpy_logging_fini(PyObject * self, PyObject * args)
{
return rclpy_detail_execute_with_logging_mutex(rclpy_logging_fini_impl, self, args);
}

/// Handle destructor for node
static void
_rclpy_destroy_node_impl(void * p)
_rclpy_destroy_node(void * p)
{
rcl_node_t * node = p;
if (!node) {
Expand All @@ -620,13 +602,6 @@ _rclpy_destroy_node_impl(void * p)
PyMem_Free(node);
}

/// See _rclpy_destroy_node_impl above.
static void
_rclpy_destroy_node(void * p)
{
rclpy_detail_execute_with_logging_mutex2(_rclpy_destroy_node_impl, p);
}

/// Create a node
/**
* Raises ValueError if the node name or namespace is invalid
Expand All @@ -639,7 +614,7 @@ _rclpy_destroy_node(void * p)
* \return NULL on failure
*/
static PyObject *
rclpy_create_node_impl(PyObject * module, PyObject * args)
rclpy_create_node(PyObject * module, PyObject * args)
{
rcl_ret_t ret;
const char * node_name;
Expand Down Expand Up @@ -752,13 +727,6 @@ rclpy_create_node_impl(PyObject * module, PyObject * args)
return node_capsule;
}

/// See rclpy_create_node_impl above.
static PyObject *
rclpy_create_node(PyObject * self, PyObject * args)
{
return rclpy_detail_execute_with_logging_mutex(rclpy_create_node_impl, self, args);
}

/// Get the name of a node.
/**
* Raises ValueError if pynode is not a node capsule
Expand Down
72 changes: 0 additions & 72 deletions rclpy/src/rclpy/detail/execute_with_logging_mutex.cpp

This file was deleted.

56 changes: 0 additions & 56 deletions rclpy/src/rclpy/detail/execute_with_logging_mutex.h

This file was deleted.

27 changes: 0 additions & 27 deletions rclpy/src/rclpy/detail/logging_mutex.cpp

This file was deleted.

47 changes: 0 additions & 47 deletions rclpy/src/rclpy/detail/logging_mutex.hpp

This file was deleted.

43 changes: 0 additions & 43 deletions rclpy/src/rclpy/detail/thread_safe_logging_output_handler.cpp

This file was deleted.

Loading

0 comments on commit 7433795

Please sign in to comment.