use rcutils_get_env instead of getenv() in node_options.cpp

[suyashb95]

use rcutils_get_env instead of getenv() in node_options.cpp
fixes ros2/ros2#856

[hidmic]

@suyash458 rcutils_get_env is supported on Windows as well, why not just using? There's a TODO right above that you'd be resolving.

[suyashb95]

Whoops, missed that one. Thanks for pointing out!

[hidmic]

Overall, I think this is an improvement, even though neither new nor old implementation are thread-safe.

There's been some discussion in ros2/rcutils#30, if you're interested in following up @suyash458. For the moment I'd be inclined to merge after CI passes. @clalancette @ivanpauno @wjwwood thoughts?

[clalancette]

There's been some discussion in ros2/rcutils#30, if you're interested in following up @suyash458. For the moment I'd be inclined to merge after CI passes. @clalancette @ivanpauno @wjwwood thoughts?

So I agree with switching to the platform-agnostic rcutils_get_env here; if and when we make that thread-safe, this will automatically benefit.

However, there is a serious bug in this patch (which I can't comment on directly, since the lines are below the ones that are changed). In the error cases, we should definitely not try to free the buffer on Windows, as that will cause this to crash (in the best case). Basically anywhere it says:

#ifdef _WIN32
      free(ros_domain_id);
#endif

in get_domain_id_from_env() is now incorrect.

[wjwwood]

even though neither new nor old implementation are thread-safe.

Is that true? I think the old method was thread-safe. On windows it gives you a copy (reason we have to free it after) and on Linux it is thread-safe so long as no one changes the environment variable (ROS_DOMAIN_ID) between reading it and using it. However, the rcutils one is not thread-safe because if anyone else uses rcutils get env the buffer returned can be overwritten on Windows because it uses a global:

https://github.com/ros2/rcutils/blob/bd590e5253672957e8c7e070523fa725a4187db2/src/get_env.c#L30

On Linux it is fine.

So, unfortunately I think this pr takes a step backward in terms of thread-safety.

[suyashb95]

even though neither new nor old implementation are thread-safe.

Is that true? I think the old method was thread-safe. On windows it gives you a copy (reason we have to free it after) and on Linux it is thread-safe so long as no one changes the environment variable (ROS_DOMAIN_ID) between reading it and using it. However, the rcutils one is not thread-safe because if anyone else uses rcutils get env the buffer returned can be overwritten on Windows because it uses a global:

https://github.com/ros2/rcutils/blob/bd590e5253672957e8c7e070523fa725a4187db2/src/get_env.c#L30

On Linux it is fine.

So, unfortunately I think this pr takes a step backward in terms of thread-safety.

@wjwwood should this wait till rcutils_get_env is thread safe on Windows?

[suyashb95]

There's been some discussion in ros2/rcutils#30, if you're interested in following up @suyash458. For the moment I'd be inclined to merge after CI passes. @clalancette @ivanpauno @wjwwood thoughts?

So I agree with switching to the platform-agnostic rcutils_get_env here; if and when we make that thread-safe, this will automatically benefit.

However, there is a serious bug in this patch (which I can't comment on directly, since the lines are below the ones that are changed). In the error cases, we should definitely not try to free the buffer on Windows, as that will cause this to crash (in the best case). Basically anywhere it says:

#ifdef _WIN32
      free(ros_domain_id);
#endif

in get_domain_id_from_env() is now incorrect.

Should I go ahead remove that check as a part of this PR?

[wjwwood]

@wjwwood should this wait till rcutils_get_env is thread safe on Windows?

Unfortunately I think that might be the case. I'd like to hear from the other reviewers first to see what they think.

I think rcutils_get_env() will need to be changed to return a copy of the string always and therefore avoid the global storage.

The other option might be to make it thread-local storage, that way ensuring it is not called more than once in the same thread before using would be safe, but that also requires allocation on new threads (implicitly), so it may not be better than just always returning a copy, other than the convenience of not needing to deallocate the returned string.

[clalancette]

I think rcutils_get_env() will need to be changed to return a copy of the string always and therefore avoid the global storage.

Yeah, I think that's the way we are going to have to go here. It's unfortunate that it blocks this PR, but otherwise we may run into more threading problems.

[hidmic]

I think the old method was thread-safe

As long as the global process environment is left unchanged as you say. No access to the environment will ever be thread-safe, we can only change the extent of the critical section.

the rcutils one is not thread-safe because if anyone else uses rcutils get env the buffer returned can be overwritten on Windows because it uses a global:

That is correct, which makes me wonder about the different implementations. As it is, rcutils_get_env() isn't reentrant on Windows but getenv isn't bound by POSIX to be reentrant either.

I think rcutils_get_env() will need to be changed to return a copy of the string always and therefore avoid the global storage.

So I fully agree with this, on all platforms. Maybe you want to take a stab at it @suyash458 ?

[suyashb95]

@hidmic Sure thing, that would close ros2/rcutils#30 right?

[clalancette]

@hidmic Sure thing, that would close ros2/rcutils#30 right?

Exactly. We also have ros2/rcutils#182 currently open, but I think we should just close that one out and make a new one that changes the contract to require the caller to free the memory. It still won't be completely thread-safe (as the environment variable could be changing while we are copying it out), but for most practical purposes it will be pretty good.

[ivanpauno]

@hidmic Sure thing, that would close ros2/rcutils#30 right?

Exactly. We also have ros2/rcutils#182 currently open, but I think we should just close that one out and make a new one that changes the contract to require the caller to free the memory. It still won't be completely thread-safe (as the environment variable could be changing while we are copying it out), but for most practical purposes it will be pretty good.

If we're going to change the contract, why not just using getenv_s directly.
It can be used in all platforms, and it's as thread safe as the current implementation of rcutils_getenv in linux, i.e.: calls to getenv_s from multiple threads are thread safe, but it's not thread safe to change the environment (using setenv, unsetenv or putenv). It requires the user to provide a buffer, but I don't think that's bad.

If we don't want to change the contract, why not just using getenv in windows too and ignore the compiler warning (the warning is just showed because windows recommends using getenv_s instead, but using getenv isn't wrong).

It still won't be completely thread-safe (as the environment variable could be changing while we are copying it out), but for most practical purposes it will be pretty good.

I would prefer a solution that's actually thread-safe (at least, for reading the environment). If not, we can get later weird issues.

[clalancette]

I would prefer a solution that's actually thread-safe (at least, for reading the environment). If not, we can get later weird issues.

So would I, but I don't actually see a way forward that accomplishes that. If we switch to getenv_s, then it is safe from interference later, but not while it is changing at the same time. We don't usually add locking to the rcutils package, so we can't actually add a lock (this would also require adding a rcutils_putenv). And using thread local storage has the problem that it can cause allocations that weren't there before. So I'm not quite sure how to make it happen.

[ivanpauno]

It still won't be completely thread-safe (as the environment variable could be changing while we are copying it out), but for most practical purposes it will be pretty good.

I think that I misread this comment before. If this is referring to avoid the global variable in windows using _dupenv_s, and copying the result of getenv in linux/mac, that would be as "thread_safe" as I mentioned before, so I'm fine with it. But I don't see the point of having a function that's not more thread safe that getenv_s, and forces an allocation (i.e.: rcutils_getenv don't add much value).
We should also add a comment in the documentation/tutorials, stating that modifying a ROS specific environment variable in runtime is not supported (or, it's not supported when the user application is multi-threaded).

[clalancette]

I think that I misread this comment before. If this is referring to avoid the global variable in windows using _dupenv_s, and copying the result of getenv in linux/mac, that would be as "thread_safe" as I mentioned before, so I'm fine with it. But I don't see the point of having a function that's not more thread safe that getenv_s, and forces an allocation (i.e.: rcutils_getenv don't add much value).

There is one reason to do it, and that's so that we can support embedded platforms that don't supply getenv_s (or any environment variable support at all). With that in place, we can just return an error if the platform doesn't support environment variables.

We should also add a comment in the documentation/tutorials, stating that modifying a ROS specific environment variable in runtime is not supported (or, it's not supported when the user application is multi-threaded).

Yeah, agreed with this.

[wjwwood]

I think rcutils_getenv should be modified to copy into a buffer, or return a newly allocated buffer (maybe both signatures), but we should still have it in rcutils so that, as @clalancette said, we can implement it ourselves, or find uses of it, or instrument it, and what have you.

[hidmic]

If we're going to change the contract, why not just using getenv_s directly.

FWIW I thought about getenv_s too, but alas there's some controversy about _s functions in general and thus support is sparse.

so that we can support embedded platforms that don't supply getenv_s

Hmm I get the concept, but I fail to see why would you do that for any C library or POSIX interface. You'd add (dummy?) support for it to your bare metal platform or RTOS of choice instead. Even basic RedHat's newlib has some stubs you can fill up.

[ivanpauno]

FWIW I thought about getenv_s too, but alas there's some controversy about _s functions in general and thus support is sparse.

Didn't know that, thanks for pointing it out.

[clalancette]

Hmm I get the concept, but I fail to see why would you do that for any C library or POSIX interface.

Well, I think the answer is in that stackoverflow report/Annex K. Namely, that glibc doesn't actually supply getenv_s, so I think we need a "wrapper" function anyway. And I'd rather not name the wrapper getenv_s, so I still think rcutils_getenv makes sense as a function we provide.

[hidmic]

@suyash458 have you been able to find the time to push this one and ros2/rcutils#30 forward?

[suyashb95]

@hidmic not yet unfortunately, I'll work on this during the weekend.

[suyashb95]

@hidmic I've submitted a PR to rcutils so rcutils_get_env returns a newly allocated string instead of using the global buffer. Lemme know if it looks fine

[ivanpauno]

closing as this doesn't apply anymore (getenv() usage was deleted)