Importer is thread-safe by contract, and JDBCStorage.ImporterImpl is not: it borrows one java.sql.Connection in its constructor and every import thread writes through that single connection and the single transaction wrapping it.
Surfaced while reviewing #884; it does not belong to that PR.
The contract
// opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/spi/Importer.java:26-32
/**
* Allows to run an import. For performance reasons, imports are run without transactions.
* <p>
* Since import is multi threaded, implementations must be thread-safe.
*/
@ThreadSafe
public interface Importer extends Closeable
What is shared
// opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:1482-1520
private final class ImporterImpl implements Importer {
final Connection con;
final ReadableTransactionImpl txr;
final WriteableTransactionTransactionImpl txw;
...
public ImporterImpl() {
...
con = getConnection();
txr = new ReadableTransactionImpl(con);
txw = new WriteableTransactionTransactionImpl(con);
}One connection, one ReadableTransactionImpl, one WriteableTransactionTransactionImpl - and with it one StampSession (:1051), which opens a second connection lazily (:407) with no synchronization of its own.
Who calls it, and from how many threads
Phase two is unconditionally parallel. invokeParallel runs one thread per chunk:
// opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/OnDiskMergeImporter.java:1522-1526
private static <V> List<V> invokeParallel(String threadNameTemplate, Collection<Callable<V>> tasks)
throws InterruptedException, ExecutionException
{
final ExecutorService executor = Executors.newCachedThreadPool(newThreadFactory(null, threadNameTemplate, true));called at :1286 with the phase-two tasks, each of which writes through the importer - ChunkCopierTask.call() (:2513-2519) copies a whole chunk with Importer.put, and DN2IDImporterTask and VLVIndexImporterTask do the same for their trees. Every tree of the backend is copied at once, through one connection.
Phase one clears the trees through it too. AbstractTwoPhaseImportStrategy.beforePhaseOne (:1346-1350) runs entryContainer.delete(asWriteableTransaction(importer)), and it is called from processEntry on the PHASE1-IMPORTER-* threads (:1248) - once per entry container, so concurrently when the backend holds more than one base DN.
Symptoms
- Statements interleave on one connection. The drivers differ in how they take that - some synchronize, some do not - but none of them makes two threads sharing one connection into two independent writers.
- One thread commits another thread's work.
clearTree (JDBCStorage.java:1160-1163), deleteTree (:1170) and openTree all end in con.commit(), so a tree being cleared in phase one commits whatever else is in flight on that connection. ImporterImpl.close() is written as though its commit() were the one that decides whether the import is durable, and it is not.
- The stamp connection can be opened twice.
StampSession.connection() (:407-412) is a plain lazy if (con==null), so two threads reaching it together open two connections and one of them is never closed - it is not the field that close() will see.
How the others do it
PDBStorage.ImporterImpl keeps its per-thread state in a ThreadLocal, which is exactly what the contract asks for:
// opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:271-280
private final class ImporterImpl implements Importer
{
private final ThreadLocal<Map<TreeName, Exchange>> exchanges = new ThreadLocal<Map<TreeName, Exchange>>()CASStorage.ImporterImpl shares one TransactionImpl, but the Cassandra CqlSession under it is thread-safe by design, so that one holds.
Suggested fix
Give each thread its own connection, borrowed on first use and returned in close() - a ThreadLocal of the transaction pair over a connection of the pool, the shape PDBStorage uses. The pool is bounded since #878, so the number of connections an import can take is bounded with it, and the phase-two thread count is what the bound has to accommodate.
The commit points want revisiting at the same time: with a connection per thread, close() has to commit each of them, not just its own.
Environment
master (5.2.x), all four JDBC dialects, import-ldif and rebuild-index. The line numbers are of master as it stands.
Importeris thread-safe by contract, andJDBCStorage.ImporterImplis not: it borrows onejava.sql.Connectionin its constructor and every import thread writes through that single connection and the single transaction wrapping it.Surfaced while reviewing #884; it does not belong to that PR.
The contract
What is shared
One connection, one
ReadableTransactionImpl, oneWriteableTransactionTransactionImpl- and with it oneStampSession(:1051), which opens a second connection lazily (:407) with no synchronization of its own.Who calls it, and from how many threads
Phase two is unconditionally parallel.
invokeParallelruns one thread per chunk:called at
:1286with the phase-two tasks, each of which writes through the importer -ChunkCopierTask.call()(:2513-2519) copies a whole chunk withImporter.put, andDN2IDImporterTaskandVLVIndexImporterTaskdo the same for their trees. Every tree of the backend is copied at once, through one connection.Phase one clears the trees through it too.
AbstractTwoPhaseImportStrategy.beforePhaseOne(:1346-1350) runsentryContainer.delete(asWriteableTransaction(importer)), and it is called fromprocessEntryon thePHASE1-IMPORTER-*threads (:1248) - once per entry container, so concurrently when the backend holds more than one base DN.Symptoms
clearTree(JDBCStorage.java:1160-1163),deleteTree(:1170) andopenTreeall end incon.commit(), so a tree being cleared in phase one commits whatever else is in flight on that connection.ImporterImpl.close()is written as though itscommit()were the one that decides whether the import is durable, and it is not.StampSession.connection()(:407-412) is a plain lazyif (con==null), so two threads reaching it together open two connections and one of them is never closed - it is not the field thatclose()will see.How the others do it
PDBStorage.ImporterImplkeeps its per-thread state in aThreadLocal, which is exactly what the contract asks for:CASStorage.ImporterImplshares oneTransactionImpl, but the CassandraCqlSessionunder it is thread-safe by design, so that one holds.Suggested fix
Give each thread its own connection, borrowed on first use and returned in
close()- aThreadLocalof the transaction pair over a connection of the pool, the shapePDBStorageuses. The pool is bounded since #878, so the number of connections an import can take is bounded with it, and the phase-two thread count is what the bound has to accommodate.The commit points want revisiting at the same time: with a connection per thread,
close()has to commit each of them, not just its own.Environment
master (5.2.x), all four JDBC dialects,
import-ldifandrebuild-index. The line numbers are of master as it stands.