Relax rmw_context_fini() API contract

[hidmic]

Relax rmw_context_fini() API contract to better match what the current implementations do and are capable of doing.

[ivanpauno]

are this being deleted because we are relaxing the requirement or because it's redundant with L132?

If the first case holds, isn't possible to ensure that in the rmw implementations?

[hidmic]

Both. Implementations complain about incorrect implementation identifier on a zero-initialized context. Even when that's not the case, guarding against partial zero-initialization (or bad initialization in the most general case) is a tough one (are init options valid to be finalized?).

[hidmic]

And I just realized we're not finalizing init options in rmw_context_t !

[ivanpauno]

Even when that's not the case, guarding against partial zero-initialization (or bad initialization in the most general case)

I think that guarding against "partially-initialized" or bad initialized is hard, but guarding against "zero-initialized" is possible.
It's not clear if a "zero-initialized" context is a "valid argument" or not, thus the docblock should be clear about that.
e.g.: zero-initialized is a valid argument for rmw_init but not for rmw_context_fini.

[hidmic]

It's not clear if a "zero-initialized" context is a "valid argument" or not, thus the docblock should be clear about that.

Doesn't it read

The context to be finalized must have been previously initialized with
rmw_init(), and then later invalidated with rmw_shutdown().

right above?

[ivanpauno]

yeap, but it's not clear what error code it returns:

• \return RMW_RET_INVALID_ARGUMENT if any arguments are invalid, or
• \return RMW_RET_INCORRECT_RMW_IMPLEMENTATION if the implementation
• identifier does not match, or

which of both apply?
should it be whatever the rmw implementation preferred? or should it be well specified?

IMO, the following code should have a specified behavior:

rmw_context_t context = rmw_get_zero_initialized_context();
rmw_ret_t ret = rmw_context_fini(context);
EXPECT_EQ(A_WELL_SPECIFIED_RETURN_CODE, ret);

and behave in the same way for all rmw implementations.
In the doc block, it's not clear if in this situation a rmw implementation returns RMW_RET_INVALID_ARGUMENT or RMW_RET_INCORRECT_RMW_IMPLEMENTATION (or at least, it's not super clear to me).

[hidmic]

should it be whatever the rmw implementation preferred? or should it be well specified?

Your comment is spot on and is directly related to ros2/rmw_implementation#107 (comment).

There's no clear definition of what an invalid context is. Common rmw implementations don't bother and, at best, simply check members for nullity. But which one goes first is undefined and thus while RMW_RET_INCORRECT_RMW_IMPLEMENTATION is the usual outcome, an rmw may equally check for the impl pointer first and return RMW_RET_INVALID_ARGUMENT. So I chose to relax the contract to match what is.

But let's say we want consistency. We can add a byte-to-byte comparison between the given context and a zero-initialized one to cover that specific kind of invalid context. The rest would remain UB. CC @brawner

[ivanpauno]

But let's say we want consistency. We can add a byte-to-byte comparison between the given context and a zero-initialized one to cover that specific kind of invalid context. The rest would remain UB. CC @brawner

Yeah, it's impossible to pretend an specific error a in partially initialized context, because it would depend in the order the rmw implementation do the checks.

I prefer consistency in the case of the zero initialized context, but I also don't mind if it's preferred to relax the requirement. In the later case, I would prefer having an explicit sentence in the docblock saying that the return code for that case is unspecified (and different to RMW_RET_OK).

[hidmic]

I thought this through a few more times. Zero-initialized contexts used anywhere but for rmw_init() is bogus code. But on the other hand, it is one of the known context states.

So I think I'll shift gears and drop this patch in favor of constraining all init/shutdown API to behave in a predictable way for those known states.

[hidmic]

See #243.

[hidmic]

CI up to test_rmw_implementation against all RMW implementations:

• Linux
• Linux-aarch64
• macOS
• Windows

[hidmic]

Closing in favor of #243.