vrozov commented on a change in pull request #1333: DRILL-6410: Memory leak in
Parquet Reader during cancellation
URL: https://github.com/apache/drill/pull/1333#discussion_r203566303
##########
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/AsyncPageReader.java
##########
@@ -284,30 +278,9 @@ protected void nextInternal() throws IOException {
}
- private void waitForExecutionResult() throws InterruptedException,
ExecutionException {
- // Get the execution result but don't remove the Future object from the
"asyncPageRead" queue yet;
- // this will ensure that cleanup will happen properly in case of an
exception being thrown
- asyncPageRead.peek().get(); // get the result of execution
- // Alright now remove the Future object
- asyncPageRead.poll();
- }
-
@Override public void clear() {
//Cancelling all existing AsyncPageReaderTasks
- while (asyncPageRead != null && !asyncPageRead.isEmpty()) {
- try {
- Future<Void> f = asyncPageRead.poll();
- if(!f.isDone() && !f.isCancelled()){
- f.cancel(true);
- } else {
- f.get(1, TimeUnit.MILLISECONDS);
- }
- } catch (RuntimeException e) {
- // Do Nothing
- } catch (Exception e) {
- // Do nothing.
- }
- }
+ executableTasksLatch.await(() -> true);
Review comment:
No, `ExecutableTasksLatch.await()` guarantees that when it returns all tasks
submitted for execution are either done or canceled. `Future.cancel()` does not
wait for the `FutureTask` to be canceled as it merely interrupts the thread
where `FutureTask` is running (in the case it is already running). Note that
after `Future` is canceled it is not possible to check whether it is finished
or not (`Future.get` throws `CancellationException`).
Missing guarantee that all tasks are finished when `clear()` returns, so
some tasks continue to run and reference vectors, so allocator reports a memory
leak.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
