Skip to content

Commit

Permalink
Spring Boot now prohibits circular references by default
Browse files Browse the repository at this point in the history
This breaks the circularity by making the executor be lazy, but that
means it can't be used during startup or shutdown of the DB connector.
Fortunately, that's also when we don't really need it; the init and
cleanup SQL are allowed to run as long as they need. See
spring-projects/spring-boot#27652 for the trigger

Also, by wrapping it we can return a constant future in the cases where
we can't actually schedule anything, making the rest of the transaction
handling a bit neater. Which is nice.
  • Loading branch information
dkfellows committed Jan 19, 2022
1 parent ff40e41 commit 797bf3f
Showing 1 changed file with 43 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import static java.util.Objects.requireNonNull;
import static java.util.concurrent.TimeUnit.NANOSECONDS;
import static java.util.stream.Collectors.toList;
import static org.apache.commons.lang3.concurrent.ConcurrentUtils.constantFuture;
import static org.slf4j.LoggerFactory.getLogger;
import static org.springframework.core.annotation.AnnotationUtils.findAnnotation;
import static org.sqlite.Function.FLAG_DETERMINISTIC;
Expand Down Expand Up @@ -88,6 +89,7 @@
import org.slf4j.Logger;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.context.annotation.Lazy;
import org.springframework.core.io.Resource;
import org.springframework.dao.DataAccessException;
import org.springframework.dao.InvalidDataAccessResourceUsageException;
Expand Down Expand Up @@ -204,9 +206,16 @@ public final class DatabaseEngine extends DatabaseCache<SQLiteConnection> {
@Autowired(required = false)
private Map<String, Function> functions = new HashMap<>();

@Lazy
@Autowired
private ScheduledExecutorService executor;

/**
* Whether to schedule a warning for long transactions. Normally want this
* when this bean is in service, but not when starting up or shutting down.
*/
private boolean warnOnLongTransactions;

// If you add more autowired stuff here, make sure in-memory DBs get a copy!

private Map<String, SummaryStatistics> statementLengths =
Expand All @@ -222,6 +231,29 @@ public final class DatabaseEngine extends DatabaseCache<SQLiteConnection> {
*/
private final ReadWriteLock lock = new ReentrantReadWriteLock();

/**
* Schedule a task (expected to be
* {@link DatabaseEngine.Connection.Locker#warnLock()}) to be run in the
* future, provided we are not initializing.
*
* @param task
* The task to schedule.
* @param nanos
* How many nanoseconds in the future this is to happen.
* @return The cancellable future. The result value of the future is
* unimportant. Never {@code null}.
*/
private Future<?> schedule(Runnable task, long nanos) {
try {
if (warnOnLongTransactions) {
return executor.schedule(task, nanos, NANOSECONDS);
}
} catch (RejectedExecutionException ignored) {
// Can't do anything about this, and it isn't too important.
}
return constantFuture(task);
}

/**
* Records the execution time of a statement, at least to first result set.
*
Expand Down Expand Up @@ -250,6 +282,7 @@ private void statementLength(Statement s, long pre, long post) {
*/
@PreDestroy
private void logStatementExecutionTimes() {
warnOnLongTransactions = false;
if (props.isPerformanceLog()) {
statementLengths.entrySet().stream()
.filter(e -> e.getValue().getMax() >= props
Expand Down Expand Up @@ -291,6 +324,8 @@ private void ensureDBsetup() {
} else {
log.debug("database {} ready", dbConnectionUrl);
}
} finally {
warnOnLongTransactions = true;
}
}

Expand Down Expand Up @@ -341,6 +376,7 @@ private DatabaseEngine(DatabaseEngine prototype) {
functions = prototype.functions;
executor = prototype.executor;
setupConfig();
warnOnLongTransactions = true;
}

/**
Expand Down Expand Up @@ -643,12 +679,8 @@ private class Locker implements AutoCloseable {
isLockedForWrites = lockForWriting;
lockingContext = getDebugContext();
l.lock();
try {
lockWarningTimeout = executor.schedule(this::warnLock,
TX_LOCK_WARN_THRESHOLD_NS, NANOSECONDS);
} catch (RejectedExecutionException ignored) {
// Can't do anything about this, and it isn't too important
}
lockWarningTimeout =
schedule(this::warnLock, TX_LOCK_WARN_THRESHOLD_NS);
lockTimestamp = nanoTime();
}

Expand All @@ -665,16 +697,18 @@ private Lock getLock(boolean lockForWriting) {
public void close() {
long unlockTimestamp = nanoTime();
currentLock.unlock();
if (lockWarningTimeout != null) {
lockWarningTimeout.cancel(false);
}
lockWarningTimeout.cancel(false);
long dt = unlockTimestamp - lockTimestamp;
if (dt > TX_LOCK_NOTE_THRESHOLD_NS) {
log.info("transaction lock was held for {}ms",
dt / NS_PER_MS);
}
}

/**
* Issue a warning that a database transaction lock has been held
* for a long time.
*/
private void warnLock() {
long dt = nanoTime() - lockTimestamp;
log.warn(
Expand Down

0 comments on commit 797bf3f

Please sign in to comment.