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.

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

avoid dynamic memory allocation during error handling #121

Merged
merged 20 commits into from
Nov 2, 2018

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Oct 29, 2018

This pull request refactors the way the error handling API in rcutils works so that it does not use dynamic memory and there does not need an allocator in most cases and also should make it easier to use in real-time and high performance situations.

Overview of major changes:

  • rcutils_error_state_t now uses fixed length strings
  • the formatted error string (rcutils_get_error_string()) returns a fixed sized string
  • error string formatting no longer uses snprintf, and instead uses custom functions which explicitly avoid memory allocations
  • there's no longer an rcutils_get_error_string_safe() as you cannot get a nullptr for the error string anymore (so it's always "safe")
  • suppression of the warning about overriding the current error now does a string compare
    • this is ok from a performance perspective since it is only checked when the error state is set (it was not cleared before setting it again)
    • this is slightly less optimal than a direct pointer comparison because it could incorrectly suppress the warning when setting the same error message in a loop (same string, not same pointer)
  • a new method rcutils_initialize_error_handling_thread_local_storage is required to be called in new threads in order to avoid memory allocations in other functions
    • it is the only function that takes an allocator now, and it is only used in certain cases
  • the pthread implementation was removed
  • I've documented the use of dynamic memory allocation for each function and tested it with the memory testing tools in osrf_testing_tools_cpp.

Main migration issues:

  • code which previously did rcutils_get_error_string_safe() will need to change to rcutils_get_error_string().str (so long as the temporary is ok)
    • to avoid the temporary, they'll need to do something like rcutils_error_string_t copy = rcutils_get_error_string();)
  • the error set macros which previously required that you pass an allocator no longer takes an allocator, e.g. change RCUTILS_SET_ERROR_MSG("char array has no valid allocator", error_msg_allocator); -> RCUTILS_SET_ERROR_MSG("char array has no valid allocator");

These changes are annoying, but I think ultimately they're for the better. I considered keeping existing signatures and doing tick-tock, and that's an option of the macro's which take allocators, but not so for the get error string signature, since it needs to return the copy to the fixed sized string. I decided against doing it for the macros because it would require using a separate name for the new macros and because deprecating them is not easy in pure C.

I have a lot more changes up and down the stack due to these API changes, and I'll open pull requests for those once I get feedback on the proposed changes here.


/cc @serge-nikulin fyi

…tion

Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood wjwwood added enhancement New feature or request in review Waiting for review (Kanban column) labels Oct 29, 2018
@wjwwood wjwwood added this to the crystal milestone Oct 29, 2018
@wjwwood wjwwood self-assigned this Oct 29, 2018
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
Copy link
Member Author

wjwwood commented Oct 29, 2018

Here's the first "downstream" pull request due to these API changes: ros2/rmw#153

Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
Copy link
Member Author

wjwwood commented Oct 30, 2018

CI to see what's missing/broken:

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

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Overall, I like the change. This initial review has some details that I came across, plus a couple of questions. I can do a bit more reviewing for it later.

include/rcutils/error_handling.h Outdated Show resolved Hide resolved
include/rcutils/error_handling.h Outdated Show resolved Hide resolved
include/rcutils/error_handling.h Show resolved Hide resolved
include/rcutils/error_handling.h Show resolved Hide resolved
include/rcutils/error_handling.h Outdated Show resolved Hide resolved
include/rcutils/error_handling.h Outdated Show resolved Hide resolved
src/error_handling_helpers.h Show resolved Hide resolved
src/error_handling_helpers.h Outdated Show resolved Hide resolved
src/error_handling_helpers.h Show resolved Hide resolved
src/error_handling.c Outdated Show resolved Hide resolved
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
…mptions

Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
Copy link
Member Author

wjwwood commented Nov 1, 2018

New CI:

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

Also, OpenSplice:

  • Linux Build Status

@wjwwood
Copy link
Member Author

wjwwood commented Nov 1, 2018

I had to rebase a few repositories due to recent merges, so here's new CI:

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

@wjwwood
Copy link
Member Author

wjwwood commented Nov 1, 2018

The only test failure was due to the launch problem. Also the job is marked as failing, but the job finished (no compiler error). @mjcarroll

@wjwwood
Copy link
Member Author

wjwwood commented Nov 1, 2018

This is waiting on ros2/launch#153 to have clean CI, but otherwise I believe CI is good for this set of pull requests and ready for review.

Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

Some minor comments, but LGTM pending CI.

src/error_handling_helpers.h Show resolved Hide resolved
src/error_handling.c Show resolved Hide resolved
src/error_handling.c Show resolved Hide resolved
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
Copy link
Member Author

wjwwood commented Nov 1, 2018

CI:

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

Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

Minor comments, feel free to ignore

include/rcutils/error_handling.h Outdated Show resolved Hide resolved
include/rcutils/error_handling.h Outdated Show resolved Hide resolved
src/error_handling.c Show resolved Hide resolved
src/error_handling_helpers.h Outdated Show resolved Hide resolved
src/error_handling_helpers.h Outdated Show resolved Hide resolved
test/test_error_handling_helpers.cpp Outdated Show resolved Hide resolved
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
Copy link
Member Author

wjwwood commented Nov 1, 2018

Ok I think I addressed all of the feedback, can you guys give it a another look.

@wjwwood
Copy link
Member Author

wjwwood commented Nov 1, 2018

Optimistic CI, pending feedback on the new changes:

  • Linux Build Status

  • Linux-aarch64 Build Status

  • macOS Build Status

  • Windows Build Status

  • Linux (OpenSplice) Build Status

Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

LGTM

src/error_handling.c Outdated Show resolved Hide resolved
@wjwwood
Copy link
Member Author

wjwwood commented Nov 2, 2018

Ok, I'm going to take my window to merge. I'm happy to follow up with changes in new pr's if needed.

@wjwwood wjwwood merged commit f8c58a1 into master Nov 2, 2018
@wjwwood wjwwood deleted the dynamic_memory_free_error_handling branch November 2, 2018 02:07
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants