Follow-up from review of #4459 (thread).
comet_c_udf_list_v1 hands the host a CometCScalarKernelList, and the host moves each CometCScalarKernel out of the array with ptr::read, then writes a Default (all callbacks None) back into the slot so the list release does not double-free.
That is correct today, but the correctness depends on the host remembering to write the default back. As @paleolimbot put it, it "seems like it would be easy to forget to do".
The suggested shape is to make the move explicit, the way the Arrow C Data Interface does it: the taker sets the source release callback to NULL and takes responsibility for the struct, and the list release then drops whatever kernels are still valid. Same net effect, but a host that forgets leaks rather than double-frees, and the contract is stated in the ABI instead of in a comment on the host side.
A related question was raised on the same code (thread): whether a failure mid-import leaks. Tracing it, it does not. Kernels already imported are owned by udfs, the one that failed is dropped with the Box passed to ImportedCScalarUdf::try_new, and the rest are freed by the list local going out of scope. A test that forces a mid-import failure would make that durable rather than incidental, and would naturally come with this change.
Note this touches the ABI struct contract, so it wants a COMET_UDF_ABI_VERSION bump if it lands after the ABI is being consumed by anyone.
Follow-up from review of #4459 (thread).
comet_c_udf_list_v1hands the host aCometCScalarKernelList, and the host moves eachCometCScalarKernelout of the array withptr::read, then writes aDefault(all callbacksNone) back into the slot so the list release does not double-free.That is correct today, but the correctness depends on the host remembering to write the default back. As @paleolimbot put it, it "seems like it would be easy to forget to do".
The suggested shape is to make the move explicit, the way the Arrow C Data Interface does it: the taker sets the source
releasecallback to NULL and takes responsibility for the struct, and the list release then drops whatever kernels are still valid. Same net effect, but a host that forgets leaks rather than double-frees, and the contract is stated in the ABI instead of in a comment on the host side.A related question was raised on the same code (thread): whether a failure mid-import leaks. Tracing it, it does not. Kernels already imported are owned by
udfs, the one that failed is dropped with theBoxpassed toImportedCScalarUdf::try_new, and the rest are freed by thelistlocal going out of scope. A test that forces a mid-import failure would make that durable rather than incidental, and would naturally come with this change.Note this touches the ABI struct contract, so it wants a
COMET_UDF_ABI_VERSIONbump if it lands after the ABI is being consumed by anyone.