Skip to content

[common] Ensure ParallelExecution waits for reader cleanup - #8855

Open
QuakeWang wants to merge 1 commit into
apache:masterfrom
QuakeWang:parallel-close
Open

[common] Ensure ParallelExecution waits for reader cleanup#8855
QuakeWang wants to merge 1 commit into
apache:masterfrom
QuakeWang:parallel-close

Conversation

@QuakeWang

Copy link
Copy Markdown
Member

Purpose

ParallelExecution marks tasks complete before closing the underlying readers, while close() previously only interrupted the executor. As a result, close() could return while reader cleanup was still running, leaving input streams temporarily
open.

Wait for executor termination in close() so reader resources are released before it returns. Add deterministic coverage for the race and remove the sleep workaround from IndexBootstrapTest.

Tests

  • mvn -pl paimon-core -am -DfailIfNoTests=false -DwildcardSuites=none -Dtest=ParallelExecutionTest,IndexBootstrapTest test
  • Ran testBootstrapIgnoresQueryAuth 1,000 times concurrently on JDK 11

ParallelExecution marks tasks complete before reader cleanup, while close only interrupts the executor. It can therefore return before input streams are released.

Wait for executor termination in close and cover the race with a controlled concurrency test.

Signed-off-by: QuakeWang <wangfuzheng0814@foxmail.com>
@JingsongLi

Copy link
Copy Markdown
Contributor

Could we avoid interrupting workers that have already finished reading and are only closing their readers?

latch.countDown() happens before the try-with-resources closes the iterator. Therefore, after take() observes completion, the worker may still be inside RecordReader.close(). Calling shutdownNow() here interrupts that cleanup. awaitTermination() then only guarantees that the worker exited; it does not guarantee that cleanup completed successfully.

I reproduced this with a reader whose close() waits on an interruptible semaphore. ParallelExecution.close() interrupted the cleanup and returned before the semaphore was released, with the cleanup operation never completed. The new test uses acquireUninterruptibly(), so it cannot expose this race.

For the normal completed-read path, could we use executorService.shutdown() when latch.getCount() == 0, and reserve shutdownNow() for early cancellation? Alternatively, moving the countdown after resource cleanup would also avoid this race.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants