Add ctx to CurrentDomain CAPI, adjust CPPAPI.#5219
Conversation
teo-tsirpanis
left a comment
teo-tsirpanis
left a comment
There was a problem hiding this comment.
Having the free functions accept a context complicates lifetime management, which has been made evident when implementing aggregates in C# was harder than the other objects. I would strongly prefer we do not do it again.
Practically speaking there are only two ways for free to fail, none of which requires a context to set an error message:
- The pointer (or the pointer it points to) is
nullptr. This is a user error that can be entirely prevented by a high-level language API. Herefreecould just return an error code, if not swallow the error at least for thefreefunctions that returnvoid. - The pointer (or the pointer it points to) is invalid. This is undefined behavior which we cannot reliably detect either way, and the best outcome here is to segfault. This is again a user error that can be entirely prevented by a high-level language API.
| * @return `TILEDB_OK` for success and `TILEDB_ERR` for error. | ||
| */ | ||
| TILEDB_EXPORT capi_return_t tiledb_current_domain_free( | ||
| tiledb_ctx_t* ctx, |
There was a problem hiding this comment.
| tiledb_ctx_t* ctx, |
There was a problem hiding this comment.
@teo-tsirpanis allocator failure would be the third option.
This was mostly a %s// job, given that it'll burden your C# work and that the generic capi handle error message is enough for the frequency I expect free() calls to fail here, I'm ok getting rid of the ctx arg.
There was a problem hiding this comment.
Are allocators expected to fail when freeing memory? I can imagine resource cleanup would be very messy if they did, akin to destructors not being allowed to throw. free does not return an error code for example.
There was a problem hiding this comment.
Opened SC-52107 to track removing the context parameter (or make it optional) from the other free APIs.
|
|
||
| void operator()(tiledb_current_domain_t* p) const { | ||
| tiledb_current_domain_free(&p); | ||
| tiledb_current_domain_free(ctx_->ptr().get(), &p); |
There was a problem hiding this comment.
The lifetime management complications I am referring to are that we need to ensure that the context must outlive the current domain and the other objects that require it to be freed.
I had managed to do that in C# (which was not without performance regressions, because freeing all objects would require multiple garbage collections unless the user explicitly disposed them), but I am not seeing it here. Deleter::ctx_ is a raw pointer, and CurrentDomain::ctx_ is a reference wrapper.
KiterLuc
changed the title
robertbindar
force-pushed
the
rbin/ch51707/add_ctx_to_currentdomain_capi
branch
from
da2cb1c to
dc33144
Compare
| * tiledb_ndrectangle_free(&ndr); | ||
| * @endcode | ||
| * | ||
| * @param ctx The TileDB context |
There was a problem hiding this comment.
| * @param ctx The TileDB context |
teo-tsirpanis
dismissed
their stale review
Context argument removed from free APIs.
As explained in #5219 (review), the requirement to pass contexts in APIs that free handles significantly increases the complexity of lifetime management in APIs for garbage-collected languages, where the objects can be finalized in any order.[^1] Because freeing these objects is trivial and can only fail if the input pointer is null[^2], we don't actually need the context to set a detailed error code. Therefore, this PR makes the context parameter optional, and updates all usages to pass `nullptr` or `NULL`, depending or the language. [^1]: For example, it's not possible to expose the aggregate APIs in TileDB-Go, without rewriting the interop system. [^2]: And if the pointer is invalid, but that's UB and we can't do anything about it. --- TYPE: C_API DESC: The context parameter in functions `tiledb_aggregate_free`, `tiledb_query_channel_free` and `tiledb_query_field_free` became unused.
3 participants
This adds a ctx parameter to all CurrentDomain CAPI calls so that internal exceptions are propagated correctly through the capi handle layer.
I also adjusted the CPP CurrentDomain API, the only current user of these calls.
[sc-51707]
TYPE: C_API | CPP_API
DESC: Add ctx to CurrentDomain CAPI, adjust CPPAPI.