paleolimbot commented on PR #702: URL: https://github.com/apache/arrow-adbc/pull/702#issuecomment-1587832450
> I would prefer that consumers explicitly hold open the connection, but it would certainly be more convenient for the PyCapsule/stream to do this implicitly. Perhaps I missed it, but I think the question is not whether the stream should close the connection/statement, but whether the stream should hold a strong reference to the Python object that keeps the memory for the `AdbcConnection`/`AdbcStatement` from being freed. If it does not hold a strong reference to that object, the failure mode could be a crash because the check for "valid connection" (`connection|statement->private_data != NULL`) involves accessing memory that has (or could have been) freed. That crash might be difficult to trigger in practice if `PyMem_Malloc()` was used (which I think is the case here). Joris noted earlier that you could also use the capsule's "context" ( https://docs.python.org/3/c-api/capsule.html#c.PyCapsule_SetContext ) instead of wrapping the stream (the documentation is a bit vague, but presumably that does the same reference increment/decrement that Joris' wrapper is doing here). I don't think you want to do that because as soon as the stream is moved to a different memory location (i.e., `RecordBatch._import_from_c()`) that reference disappears. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
