Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add ctx to CurrentDomain CAPI, adjust CPPAPI. #5219

Merged
merged 4 commits into from
Aug 8, 2024

Conversation

robertbindar
Copy link
Contributor

@robertbindar robertbindar commented Jul 29, 2024

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.

Copy link
Member

@teo-tsirpanis teo-tsirpanis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. Here free could just return an error code, if not swallow the error at least for the free functions that return void.
  • 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.

* @param current_domain The current domain to be freed
* @return `TILEDB_OK` for success and `TILEDB_ERR` for error.
*/
TILEDB_EXPORT capi_return_t tiledb_current_domain_free(
tiledb_ctx_t* ctx,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tiledb_ctx_t* ctx,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened SC-52107 to track removing the context parameter (or make it optional) from the other free APIs.

@@ -118,11 +118,11 @@ class Deleter {
}

void operator()(tiledb_current_domain_t* p) const {
tiledb_current_domain_free(&p);
tiledb_current_domain_free(ctx_->ptr().get(), &p);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 KiterLuc changed the title Add ctx to CurrentDomain CAPI, adjust cppapi Add ctx to CurrentDomain CAPI, adjust CPPAPI. Jul 30, 2024
@robertbindar robertbindar force-pushed the rbin/ch51707/add_ctx_to_currentdomain_capi branch from da2cb1c to dc33144 Compare August 5, 2024 13:54
@@ -86,6 +86,7 @@ TILEDB_EXPORT capi_return_t tiledb_ndrectangle_alloc(
* tiledb_ndrectangle_free(&ndr);
* @endcode
*
* @param ctx The TileDB context
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param ctx The TileDB context

@teo-tsirpanis teo-tsirpanis dismissed their stale review August 5, 2024 13:56

Context argument removed from free APIs.

Copy link
Member

@teo-tsirpanis teo-tsirpanis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@KiterLuc KiterLuc merged commit b8ca549 into dev Aug 8, 2024
61 checks passed
@KiterLuc KiterLuc deleted the rbin/ch51707/add_ctx_to_currentdomain_capi branch August 8, 2024 10:27
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants