Skip to content

[Fix][Connector-V2] Fix Arrow memory leak by correcting close order in ArrowToSeatunnelRowReader#10958

Open
davidzollo wants to merge 1 commit into
apache:devfrom
davidzollo:fix-arrow-memory-leak-close-order
Open

[Fix][Connector-V2] Fix Arrow memory leak by correcting close order in ArrowToSeatunnelRowReader#10958
davidzollo wants to merge 1 commit into
apache:devfrom
davidzollo:fix-arrow-memory-leak-close-order

Conversation

@davidzollo
Copy link
Copy Markdown
Contributor

Purpose

Fix IllegalStateException: Memory was leaked by query when using the Doris connector (and any connector backed by ArrowToSeatunnelRowReader).

Closes #9863

Problem

In ArrowToSeatunnelRowReader.close(), the previous close order was:

  1. root.close() — close VectorSchemaRoot
  2. rootAllocator.close() — close RootAllocatorwrong: arrowStreamReader still holds unreleased buffers
  3. arrowStreamReader.close() — too late, allocator already closed

Apache Arrow's memory management requires that all child allocations are released before the RootAllocator is closed. The ArrowStreamReader internally allocates Arrow buffers via the RootAllocator. Closing the allocator while the reader still holds references causes the allocator to detect leaked memory (~64 bytes of internal metadata) and throw:

java.lang.IllegalStateException: Memory was leaked by query. Memory leaked: (64)
Allocator(ROOT) 0/64/64/2147483647 (res/actual/peak/limit)

This exception propagated up through DorisValueReader.hasNext()DorisSourceReader.pollNext()SourceFlowLifeCycle.collect() causing the task to fail with state FAILED.

Fix

Corrected the close order to:

  1. arrowStreamReader.close() — releases all Arrow buffers and internally closes VectorSchemaRoot
  2. rootAllocator.close() — now safe, all allocations already released

The separate root.close() call is removed since ArrowStreamReader.close() already handles it.

Impact

  • Affected connectors: Doris, StarRocks, and any connector using ArrowToSeatunnelRowReader
  • Trigger: Any read operation that creates an ArrowToSeatunnelRowReader instance
  • Backward compatible: Yes, pure bug fix with no API or behavior changes

…n ArrowToSeatunnelRowReader

Close arrowStreamReader before rootAllocator to prevent memory leak.
ArrowStreamReader internally closes VectorSchemaRoot and releases all
Arrow buffer allocations back to the allocator. The previous order closed
rootAllocator while arrowStreamReader still held unreleased buffers,
causing IllegalStateException: Memory was leaked by query (issue apache#9863).

Fix: apache#9863
Copy link
Copy Markdown
Contributor

@DanielLeens DanielLeens left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. I reviewed the full diff on the current head and retraced the close path instead of only reading the PR description.

What this PR fixes

  • User pain: the Arrow reader can throw a leaked-memory exception during shutdown even after the read path itself succeeds.
  • Fix approach: close ArrowStreamReader before closing the root allocator.
  • One-line summary: this fixes the real Arrow resource-release order on the normal close path.

Runtime chain I checked

Arrow source read path
  -> ArrowStreamReader owns VectorSchemaRoot buffers
  -> ArrowToSeatunnelRowReader.close()
      -> ArrowStreamReader.close()
      -> VectorSchemaRoot buffers released back to allocator
      -> RootAllocator.close()

Findings

  • The normal close path definitely hits ArrowToSeatunnelRowReader.close().
  • The new order at ArrowToSeatunnelRowReader.java:320-334 is the correct one for Arrow ownership semantics.
  • I do not see a reopened source-level blocker on this revision.

Merge conclusion

Conclusion: can merge after fixes

  1. Blocking items
  • No code blocker from my side.
  • Please let the latest GitHub Build finish green before merging.
  1. Suggested non-blocking follow-up
  • None from my side on the source path.

Overall, this is a focused and correct fix for the allocator-leak shutdown path.

@nzw921rx
Copy link
Copy Markdown
Collaborator

+1

@DanielLeens
Copy link
Copy Markdown
Contributor

Thanks for taking a look, @nzw921rx. On the unchanged head 25744b5, Daniel's source-level conclusion is still the same as before: I do not see a reopened code blocker on this revision, and from my side the remaining item is still to let the latest GitHub Build finish green before merge.

There is no new code on the PR head, so I am not starting another full review in this round. Happy to re-check if the branch changes again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory leak exception occurs when reading data with the Doris connector

3 participants