GH-35988: [C#] The C data interface implementation can leak on import

[CurtHagenlocher]

What changes are included in this PR?

To ensure proper cleanup, immediately copies the contents of the C structure into the imported class for arrays and streams.
Relaxes the requirement when exporting that the target structure appear uninitialized.

Are these changes tested?

Existing tests pass. We don't as yet seem to have a good way to test for memory leaks so no new tests have been added.

Are there any user-facing changes?

No.

• Closes: [C#] The C data interface implementation can leak on import #35988

[github-actions]

⚠️ GitHub issue #35988 has been automatically assigned in GitHub to PR creator.

[pitrou]

Hmm, so there are two (unrelated?) changes here.

The first change (removing the cArray->release != null checks in export functions) looks fine.

The second change is IMHO the symptom of an implementation issue. ImportedArrowArray currently stores the given raw pointer CArrowArray* _cArray. Instead, it should probably store its own CArrowArray struct like the C++ importer does:

arrow/cpp/src/arrow/c/bridge.cc

Lines 1241 to 1261 in e920bed

	// A wrapper struct for an imported C ArrowArray.
	// The ArrowArray is released on destruction.
	struct ImportedArrayData {
	struct ArrowArray array_;
	ImportedArrayData() {
	ArrowArrayMarkReleased(&array_); // Initially released
	}
	void Release() {
	if (!ArrowArrayIsReleased(&array_)) {
	ArrowArrayRelease(&array_);
	DCHECK(ArrowArrayIsReleased(&array_));
	}
	}
	~ImportedArrayData() { Release(); }
	private:
	ARROW_DISALLOW_COPY_AND_ASSIGN(ImportedArrayData);
	};

Then, importing an array moves the user-supplied structure to the owned structure, like this:

arrow/cpp/src/arrow/c/bridge.cc

Lines 1285 to 1287 in e920bed

	import_ = std::make_shared<ImportedArrayData>();
	c_struct_ = &import_->array_;
	ArrowArrayMove(src, c_struct_);

Therefore, the imported array structure does not depend on the lifetime of the user-supplied pointer.

By the way, moving is cheap so you shouldn't fear any inefficiency here:

arrow/cpp/src/arrow/c/helpers.h

Lines 66 to 75 in e920bed

	/// Move the C array from `src` to `dest`
	///
	/// Note `dest` must *not* point to a valid array already, otherwise there
	/// will be a memory leak.
	inline void ArrowArrayMove(struct ArrowArray* src, struct ArrowArray* dest) {
	assert(dest != src);
	assert(!ArrowArrayIsReleased(src));
	memcpy(dest, src, sizeof(struct ArrowArray));
	ArrowArrayMarkReleased(src);
	}

[CurtHagenlocher]

The second change is IMHO the symptom of an implementation issue. ImportedArrowArray currently stores the given raw pointer CArrowArray* _cArray. Instead, it should probably store its own CArrowArray struct like the C++ importer does:

Ah, that's much nicer. You can tell I haven't been responsible for managing my own resources in a few years... .

[adamreeve]

For what it's worth, it looks like this change fixes an issue I ran into using a C# exported stream from an older version of C++ Arrow, where the schema struct used when importing the stream was uninitialized: https://github.com/apache/arrow/blob/apache-arrow-10.0.1/cpp/src/arrow/c/bridge.cc#L1782 (this has since been fixed)

[westonpace]

If I understand correctly the basic premise is that, on import, we let CArrowArray be in the garbage collector pool since it should be safe to destroy if we happen to lose all references to it (unlike an exported array which we assume will have external references)? That seems valid to me.

[CurtHagenlocher]

If I understand correctly the basic premise is that, on import, we let CArrowArray be in the garbage collector pool since it should be safe to destroy if we happen to lose all references to it (unlike an exported array which we assume will have external references)? That seems valid to me.

By embedding in the ImportedArrowArray, it's allocated as part of that object and will have a lifetime equal to it. This is exactly what we want, because it's the ImportedArrowArray which already manages the lifetime of the imported memory.

[pitrou]

As far as I can understand, the code looks good now (but I'm not a C# developer).

[eerhardt]

LGTM. I'll merge this soon, if I don't hear any objections.

[conbench-apache-arrow]

Conbench analyzed the 5 benchmark runs on commit 1e2dfceb.

There was 1 benchmark result indicating a performance regression:

• Commit Run on ursa-thinkcentre-m75q at 2023-06-16 07:27:47Z
  • params=<SMALL_VECTOR(std::shared_ptr)>, source=cpp-micro, suite=arrow-small-vector-benchmark

The full Conbench report has more details.