eclipse-zenoh / zenoh-c

C API for Zenoh
http://zenoh.io
Other
74 stars 54 forks source link

resolve `z_check` ambiguity #492

Closed milyin closed 2 weeks ago

milyin commented 2 months ago

Describe the release item

The z_xxx_check is pair function to z_xxx_null. It should return "true" if object is in a "null" state. The z_xxx_null creates "stub" owned object which is 1) safe to be forgotten without explicit dropping. This is the state which object is set after z_drop operation 2) may be unusable, e.g. z_loan on it may crash

The problem is that for some objects this null/stub state is valid. E.g. empty buffer or default encoding.

The proposal is to make this interface change:

Also it makes sense to get rid of active usage of "z_check" in examples - in most cases error code from constructor is enough

DenisBiryukov91 commented 1 month ago

for non-blocking channels it is necessary to call z_check on objects returned by try_recv()

milyin commented 1 month ago

for non-blocking channels it is necessary to call z_check on objects returned by try_recv()

Ok, I see the problem. Separating z_check to z_is_empty/z_check may provoke more problems due to their misusage. I think now that it's better to keep only one z_check function. It should return true when and only when the object is in the same state, as it would have been created by z_xxx_null. It doesn't matter if the object is actually usable or not.

But the problem remains. The z_loan is unsafe operation now: there is no null check in it to make code faster. But z_check is not a complementary operation for z_loan: it returns false on empty objects which actually can be loaned. So we don't have a way to check if object can be loaned or not.

So I have another proposal which doesn't require API change right now. Let's make z_loan safe: it should return NULL when loaning null owned object. In most cases it shouldn't affect performance. For rare cases when it does we can later provide z_loan_unsafe which doesn't do this check. @yellowhatter , @DenisBiryukov91 , @sashacmc what do you think?

milyin commented 1 month ago

After discussion with @DenisBiryukov91 the proposal is to make z_check usable on loaned objects. This means that even for objects in gravestone state the z_loan is allowed. Though I still not completely sure what's the role of z_check in this case. Should it return true or false on slice with null data pointer for example? Should it always return true for Encoding which is always valid? And we still lack for the way for checking if the owned object is in gravestone state or not.

Another solution is to declare that z_check is a function complementary to z_null and it returns true when and only when the object is in the "gravestone" state (which is result of z_null or z_drop or failed constructor). There is no way to check if the object loanable or not, this depends on object. This is a hole in the API, but it can be resolved later if necessary

milyin commented 1 month ago

After discussion with @DenisBiryukov91 and discussion between @DenisBiryukov91 and @Mallets as far as I understand the decision to make z_check and z_null undocumented was made. Please correct me if I'm wrong.

Though this raises question about our guarantees about double drop safety and about gravestone state of objects in case of failed construction. z_null is used to set owned object to gravestone state. z_check tests if object is in this state. Without these functions there is no way to check for these guarantees.

The double-drop guarantee and the gurantee that failed constructor leaves owned object in safe to drop state are necessary for C++ wrapper. So we have to provide this state and test for this state anyway.

So I see no reason to hide this state from user and I think that z_null and z_check should remain

milyin commented 1 month ago

After discussion with @Mallets the final decision is to rename them to _z_check and _z_null to keep them out of official API but still use in tests for check for our double drop guarantees