Modify rcutils_getenv to be thread safe

[ivanpauno]

Complements ros2/rcl#502

Documentation about thread local storage:

• https://gcc.gnu.org/onlinedocs/gcc-3.3/gcc/Thread-Local.html
• https://docs.microsoft.com/en-us/cpp/cpp/thread?view=vs-2019
• https://clang.llvm.org/docs/AttributeReference.html#id289

[wjwwood]

rcutils/include/rcutils/macros.h

Line 33 in e52f2cf

#define RCUTILS_THREAD_LOCAL __declspec(thread)

?

[ivanpauno]

It's the windows specific way of declaring a variable thread local. See https://docs.microsoft.com/en-us/cpp/cpp/thread?view=vs-2019.

[ivanpauno]

Oh, I see we already had this

[wjwwood]

This may cause memory allocations where before they did not occur, see:

rcutils/include/rcutils/error_handling.h

Lines 101 to 141 in e70143a

	/// Forces initialization of thread-local storage if called in a newly created thread.
	/**
	* If this function is not called beforehand, then the first time the error
	* state is set or the first time the error message is retrieved, the default
	* allocator will be used to allocate thread-local storage.
	*
	* This function may or may not allocate memory.
	* The system's thread-local storage implementation may need to allocate
	* memory, since it usually has no way of knowing how much storage is needed
	* without knowing how many threads will be created.
	* Most implementations (e.g. C11, C++11, and pthread) do not have ways to
	* specify how this memory is allocated, but if the implementation allows, the
	* given allocator to this function will be used, but is otherwise unused.
	* This only occurs when creating and destroying threads, which can be avoided
	* in the "steady" state by reusing pools of threads.
	*
	* It is worth considering that repeated thread creation and destruction will
	* result in repeated memory allocations and could result in memory
	* fragmentation.
	* This is typically avoided anyways by using pools of threads.
	*
	* In case an error is indicated by the return code, no error message will have
	* been set.
	*
	* If called more than once in a thread, or after implicitly initialized by
	* setting the error state, it will still return `RCUTILS_RET_OK`, even
	* if the given allocator is invalid.
	* Essentially this function does nothing if thread-local storage has already
	* been called.
	* If already initialized, the given allocator is ignored, even if it does not
	* match the allocator used originally to initialize the thread-local storage.
	*
	* \return `RCUTILS_RET_OK` if successful, or
	* \return `RCUTILS_RET_INVALID_ARGUMENT` if the allocator is invalid, or
	* \return `RCUTILS_RET_BAD_ALLOC` if allocating memory fails, or
	* \return `RCUTILS_RET_ERROR` if an unspecified error occurs.
	*/
	RCUTILS_PUBLIC
	RCUTILS_WARN_UNUSED
	rcutils_ret_t
	rcutils_initialize_error_handling_thread_local_storage(rcutils_allocator_t allocator);

[hidmic]

Hmm, the fix itself is a fair solution, but I'm curious about rcutils_get_env. It wraps getenv_s when compiled for Windows and getenv for all the other platforms. But then, getenv_s ought to be available in all platforms as it is included in the C11 standard. Why not simply use getenv_s where appropriate?

[wjwwood]

getenv_s requires that you use your own storage, whereas getenv returns just a pointer to it.

Also, neither is thread-safe as far as I know:

This function is not required to be thread-safe. Another call to getenv, as well as a call to the POSIX functions setenv(), unsetenv(), and putenv() may invalidate the pointer returned by a previous call or modify the string obtained from a previous call.

https://en.cppreference.com/w/c/program/getenv

I'd recommend reading all of #30 if you haven't already.

[hidmic]

Alright, I see now. And I think the existing one is the best solution. In light of this, should #30 be closed as won't do?

[wjwwood]

In light of this, should #30 be closed as won't do?

I'm not sure. I think it's definitely not needed now, but maybe others have an opinion about it. We could close it or leave it open, doesn't matter to me. If we close it we can always reopen it.

[clalancette]

I think in light of what we discussed in ros2/rclcpp#987 (comment) , we should close this out in favor of a new PR. I'm going to go ahead and do that, but feel free to re-open if you disagree.