Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.apache.paimon.types.DataType;
import org.apache.paimon.types.FloatType;
import org.apache.paimon.types.VectorType;
import org.apache.paimon.utils.IOUtils;

import org.aliyun.lumina.LuminaDataset;
import org.aliyun.lumina.LuminaFileOutput;
Expand Down Expand Up @@ -427,16 +428,23 @@ static class FileBackedDataset implements LuminaDataset, Closeable {
FileBackedDataset(File file, int dim, long totalCount, String phase, int bufferSize)
throws IOException {
this.raf = new RandomAccessFile(file, "r");
this.channel = raf.getChannel();
this.dim = dim;
this.totalCount = totalCount;
this.recordSizeInBytes = checkedRecordSize(dim, bufferSize);
this.cursor = 0;
this.readBuf = ByteBuffer.allocateDirect(bufferSize);
this.readBuf.order(ByteOrder.nativeOrder());
this.readBuf.limit(0); // empty initially
this.phase = phase;
this.lastLoggedPercent = -1;
try {
this.channel = raf.getChannel();
this.dim = dim;
this.totalCount = totalCount;
this.recordSizeInBytes = checkedRecordSize(dim, bufferSize);
this.cursor = 0;
this.readBuf = ByteBuffer.allocateDirect(bufferSize);
this.readBuf.order(ByteOrder.nativeOrder());
this.readBuf.limit(0); // empty initially
this.phase = phase;
this.lastLoggedPercent = -1;
} catch (RuntimeException e) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] Also close the file when direct-buffer allocation throws an Error

The cleanup currently only handles RuntimeException, but the other post-open failure named in the PR, ByteBuffer.allocateDirect, can throw OutOfMemoryError. A direct-memory OOM can be caught by a higher-level task/runtime and leave this JVM alive; in that path the constructor still strands raf, which is exactly the resource-safety hole this change is intended to close. Please catch and rethrow RuntimeException | Error (or use the usual constructor-cleanup catch (Throwable) pattern) so every failure after the successful open releases the handle. The existing regression only exercises checkedRecordSize, so it does not cover this path.

// a caller's try-with-resources never sees an object whose construction failed,
// so this is the only chance to release the file handle
IOUtils.closeQuietly(raf);
throw e;
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.paimon.lumina.index;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;

import java.io.File;
import java.io.FileOutputStream;
import java.nio.file.Path;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.jupiter.api.Assumptions.assumeTrue;

/**
* Tests that {@link LuminaVectorGlobalIndexWriter.FileBackedDataset} releases the file it opened
* when the rest of its construction fails.
*/
public class LuminaFileBackedDatasetCloseTest {

@TempDir Path tempDir;

/**
* checkedRecordSize rejects a dimension whose record does not fit the read buffer, and it runs
* after the file is open. A caller's try-with-resources never sees an object whose constructor
* threw, so nothing would close that handle.
*/
@Test
public void testFailedConstructionReleasesTheFile() throws Exception {
File fdDir = new File("/dev/fd");
assumeTrue(fdDir.isDirectory() && fdDir.list() != null, "needs /dev/fd");

File file = new File(tempDir.toFile(), "vectors.bin");
try (FileOutputStream out = new FileOutputStream(file)) {
out.write(new byte[64]);
}

int before = fdDir.list().length;
for (int i = 0; i < 200; i++) {
assertThatThrownBy(
() ->
new LuminaVectorGlobalIndexWriter.FileBackedDataset(
file, Integer.MAX_VALUE / 4, 1L, "test", 4096))
.isInstanceOf(IllegalStateException.class);
}
int after = fdDir.list().length;

assertThat(after - before)
.as("200 failed constructions must not strand 200 descriptors")
.isLessThan(50);
}
}
Loading