From 2a7bb9d7eda865dbf5fca3ca94a33c325e7ab6e5 Mon Sep 17 00:00:00 2001
From: Maxim Thomas <maxim.thomas@gmail.com>
Date: Wed, 09 Sep 2026 07:25:51 +0000
Subject: [PATCH] [#885] Give a connection of the JDBC pool a read bound of its own, and take it off for a statement that carries none (#934)
---
opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java | 27 +
opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CachedConnectionTestCase.java | 258 +++++++++++++++
opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStatementBoundTestCase.java | 242 ++++++++++++++
opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java | 209 +++++++++++-
opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java | 242 ++++++++++++-
5 files changed, 933 insertions(+), 45 deletions(-)
diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java
index acb9e86..c03b8f6 100644
--- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java
+++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java
@@ -112,6 +112,98 @@
static final String POOL_TIMEOUT_PROPERTY = "org.openidentityplatform.opendj.jdbc.pool.timeout";
static final long DEFAULT_POOL_TIMEOUT_SECONDS = 60;
+ /**
+ * The socket read timeout a connection of this pool carries for the whole of its life, in
+ * seconds; 0 for none, which is what every connection of this backend carried before this
+ * property existed, and the default.
+ * <p>
+ * It bounds what neither of the two bounds before it reaches. The bound of
+ * {@value #CONNECT_TIMEOUT_PROPERTY} covers the connect and the login and is taken off as soon
+ * as the login is through, and the socket read timeout {@code JDBCStorage} arms behind a
+ * cancelled statement is armed for the length of that statement alone - so a commit, a
+ * rollback, a lookup of the catalog and the rows a cursor drains are all read from a socket
+ * with no deadline of any kind, and an operation that meets a database which stopped answering
+ * after the login stays parked.
+ * <p>
+ * The default is a consequence rather than caution: a bound standing on the connection has to
+ * exceed the longest silence the database may legitimately produce, and the class the longest
+ * statements belong to ({@code JDBCStorage.StatementBound.BULK}) ships unbounded for reasons of
+ * its own. There is no value here that does not contradict it - so the deployment that knows how
+ * long its database may go without answering is the one that sets this, and the statements of
+ * that class run with it taken off for as long as they do.
+ * <p>
+ * That silence is what this is sized against, rather than the longest statement, because the
+ * lift covers statements alone: the commit at the end of an import is a call of its own with no
+ * statement in flight behind it - {@code ImporterImpl.close()} commits a whole import in one -
+ * and so are the rollback of every borrow and the reads of the catalog. Every one of them runs
+ * under this bound whatever the class of the statements before it. It has to exceed the bound of
+ * an ordinary statement as well ({@code JDBCStorage.StatementBound.OPERATION}, two minutes by
+ * default): under it, such a statement dies on the socket at this value instead of being
+ * cancelled at its own, which costs the connection the driver then closes and reports neither
+ * property. A backend opening with the two set that way says so once.
+ * <p>
+ * A read bound standing in the connection string is the deployment's own and is left alone by
+ * every part of this: it is not replaced here, and it is not the one taken off there. A value of
+ * 0 there is no bound of theirs, though - it is the default of the driver, written out - and
+ * this one goes on top of it.
+ */
+ static final String READ_TIMEOUT_PROPERTY = "org.openidentityplatform.opendj.jdbc.read.timeout";
+ static final int DEFAULT_READ_TIMEOUT_SECONDS = 0;
+
+ // Read once, at class initialization, for the reason aliveBypassNanos is: it is read on every
+ // connect and on every statement that has to take it off again, and neither is the place to
+ // parse a system property. Not final so that a test can vary it without a class loader of its
+ // own, and volatile because a non-final static is written neither atomically nor visibly to the
+ // threads reading it (JLS 17.7).
+ static volatile int readTimeoutMillis = getReadTimeoutMillis();
+
+ /**
+ * The bound of {@value #READ_TIMEOUT_PROPERTY} in milliseconds, which is the unit
+ * {@code setNetworkTimeout} takes. A value that is not a number, and a negative one, are
+ * reported once and ignored in favour of the default - a deployment that asked for a bound and
+ * misspelled it gets none, and that is the one thing this property exists to keep from
+ * happening quietly. A value past {@link JDBCStorage#MAX_BOUND_SECONDS} is taken down to it: the
+ * ceiling there is what a socket read timeout can hold at all, and a value beyond it would reach
+ * the driver as a negative timeout - outside the contract of the call, and a value a driver is
+ * free to read as anything.
+ */
+ static int getReadTimeoutMillis() {
+ // Clamped against the ceiling rather than through JDBCStorage.clampSeconds(): that ceiling is
+ // a compile-time constant and reaches this class inlined, while the call would be a
+ // package-private call into another class - and this runs in the initializer of this one,
+ // which is loaded by whatever loader defines it. Across two loaders that is an
+ // IllegalAccessError rather than a call, and it would leave the class uninitializable.
+ final long seconds = Math.min(JDBCStorage.MAX_BOUND_SECONDS,
+ getNonNegativeProperty(READ_TIMEOUT_PROPERTY, DEFAULT_READ_TIMEOUT_SECONDS, "s"));
+ return (int) (seconds * 1000);
+ }
+
+ /**
+ * The read bound this class put on a connection of this url, in milliseconds, or 0 for a
+ * connection carrying none of ours. What {@code JDBCStorage} has to know before it takes that
+ * bound off for a statement of a class that carries no bound of its own: a read timeout
+ * standing in the connection string is the deployment's own, and a driver whose property names
+ * are not known here was never given one - taking either off would leave the connection
+ * unbounded for the rest of its life in the pool, which is a bound taken away from a deployment
+ * that asked for one.
+ * <p>
+ * The dialect is read off the connection string here, the way {@link #getConnection} reads the
+ * one it hands {@link #connect}: the two have to answer the same, or the bound taken off would
+ * not be the bound that was set.
+ */
+ static int standingReadBoundMillis(String connectionString) {
+ return connectionString == null ? 0
+ : standingReadBoundMillis(connectionString, ConnectDialect.of(connectionString));
+ }
+
+ private static int standingReadBoundMillis(String connectionString, ConnectDialect dialect) {
+ final int millis = readTimeoutMillis;
+ if (millis <= 0 || dialect == null || dialect.bounds(connectionString, dialect.readProperties)) {
+ return 0;
+ }
+ return millis;
+ }
+
/** Bound of the validation of a pooled connection: isValid(0) means "no timeout" in the JDBC contract. */
static final int VALIDATION_TIMEOUT_SECONDS = 5;
@@ -930,6 +1022,46 @@
return false;
}
+ /**
+ * Whether one of these is bounded by the connection string, or by a system property this
+ * driver reads, as the bound of an established connection has to ask it. Told apart from
+ * {@link #declared} by what a 0 means: there the question is whether a property of ours is
+ * to be supplied to the connect at all, and on postgresql a parameter of the url outranks
+ * that property whatever it says - while a bound put on an established connection with
+ * setNetworkTimeout is outranked by nothing, so a "socketTimeout=0" is no bound of the
+ * deployment's to stay out of the way of. It is the default of the driver, written out, and
+ * reading it as theirs would leave a deployment that asked for a standing bound with none.
+ * <p>
+ * The names are recognized in the url the way {@link #declared} recognizes them, and out of
+ * the system properties from the same list - see the comment on that method.
+ */
+ private boolean bounds(String connectionString, String... properties) {
+ for (final String property : properties) {
+ if (boundInUrl(connectionString, property) || setAsSystemProperty(property)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * Whether the connection string bounds this property, under its own name or the last segment
+ * of it. The two names are asked independently, the way {@link #declaredInUrl} asks them:
+ * stopping at the first name present, even where its value is a zero, let a
+ * "...?oracle.jdbc.ReadTimeout=0&ReadTimeout=600" answer with the zero of the name that comes
+ * first and hide the bound standing behind it. Such a url is declared() and would then not
+ * be bounds(): the login keeps the administrator's 600 s and one of ours goes on top of it
+ * with setNetworkTimeout. The two predicates have to look at the same set of names for "the
+ * bound taken off is the bound that was set" to hold.
+ */
+ private boolean boundInUrl(String connectionString, String property) {
+ if (isBound(parameterValue(connectionString, property))) {
+ return true;
+ }
+ final int dot = property.lastIndexOf('.');
+ return dot >= 0 && isBound(parameterValue(connectionString, property.substring(dot + 1)));
+ }
+
// Whether the administrator bounded one of these properties themselves. The dialects
// separate their parameters differently - "?a=1&b=2" (postgresql, mysql), ";a=1;b=2" (sql
// server), "(A=1)" inside the descriptor of an oracle tns url, where the property also goes
@@ -1237,6 +1369,13 @@
* are the ones of a driver, so a driver outside the four leaves every attempt unbounded - and
* the deadline of the borrow cannot reach into a connect that is already under way, since the
* driver is the only thing holding the socket.
+ * <p>
+ * The connect is not the whole of it. {@value #READ_TIMEOUT_PROPERTY} is not put on the
+ * connections of such a pool either: {@link #standingReadBoundMillis(String)} answers 0 for a
+ * dialect this class does not know, so a read timeout the url may already carry under a name of
+ * its own is left alone rather than covered by one of ours. That silence is what this says out
+ * loud, since the strict parsing of that property exists precisely so that a deployment which
+ * asked for a bound is never quietly left with none.
*/
private static void reportUnknownDialect(String connectionString, ConnectDialect dialect) {
if (dialect != null) {
@@ -1246,11 +1385,19 @@
for (final ConnectDialect candidate : ConnectDialect.values()) {
known.append(known.length() > 0 ? ", " : "").append(candidate.urlPrefix);
}
+ // Only where one was asked for: a deployment running on the default of that property asked
+ // for no standing bound anywhere, and has nothing to act on here.
+ final String standingBound = readTimeoutMillis > 0
+ ? ", and the " + READ_TIMEOUT_PROPERTY + " asked for is not put on the connections of this pool"
+ + " either - a read of one whose database stops answering after the login waits with no deadline"
+ + " able to reach it. Such a url may bound the read under a name this backend does not know, which"
+ + " is why none is set on top of it: bound it in the url instead"
+ : "";
warnOnce(safeUrl(connectionString) + "|unknown-dialect",
"%s names a driver whose timeout properties are not known to this backend (%s are): a connect to a"
+ " database that accepts it and does not answer is left without a bound, and the %s property"
- + " cannot end it",
- safeUrl(connectionString), known, POOL_TIMEOUT_PROPERTY);
+ + " cannot end it%s",
+ safeUrl(connectionString), known, POOL_TIMEOUT_PROPERTY, standingBound);
}
/**
@@ -1441,12 +1588,10 @@
// still under the read bound: both of these are round trips of their own
conNew.setAutoCommit(false);
conNew.setTransactionIsolation(TRANSACTION_READ_COMMITTED);
- if (readBoundSet) {
- // a driver that will not take the bound back has warned about it already: the
- // connection serves the borrower that is waiting for it and is closed rather than
- // pooled, so the bound of the login does not outlive it in the pool
- poolable = relaxReadBound(conNew, connectTimeoutSeconds);
- }
+ // whatever the driver will not take here it has warned about already: a connection left
+ // carrying the read bound of its login serves the borrower that is waiting for it and is
+ // closed rather than pooled, so that bound does not outlive it in the pool
+ poolable = applyStandingReadBound(conNew, connectionString, dialect, connectTimeoutSeconds, readBoundSet);
} catch (SQLException | RuntimeException e) { // nothing holds this connection yet: it would leak
closeQuietly(conNew);
throw e;
@@ -1456,19 +1601,41 @@
return established;
}
- // The second bound of the login is a socket read timeout on mysql, oracle and sql server, in
- // force for the whole life of the connection: left in place it would break every statement
- // slower than it - an import batch, the statistics of a freshly loaded table - so it is lifted
- // as soon as the login is through, restoring the behaviour of a connection this class
- // established before. A read bound the connection string sets itself is never touched here:
- // it is not set at all, so nothing of the administrator's is lifted along with it. Returns
- // whether the bound is gone - a connection still carrying it must not be pooled.
- // Named by the bound the login was given rather than by the property it came from: with
- // CONNECT_TIMEOUT_PROPERTY at 0 the attempt takes its bound from what is left of the deadline
- // of the borrow, so naming that property would point at the one setting that is not in force.
- private static boolean relaxReadBound(Connection con, long boundSeconds) {
- return setNetworkTimeout(con, 0, "statements taking longer than the " + boundSeconds
- + "s the login of this connection was bounded by fail on it, and it is closed rather than pooled");
+ /**
+ * Gives an established connection the read bound it carries from here on, which is the same
+ * call that takes the read bound of its login off.
+ * <p>
+ * The second is not optional. On mysql, oracle and sql server the second bound of the login is
+ * a socket read timeout in force for the whole life of the connection, and left in place it
+ * breaks every statement slower than a connect - an import batch, the statistics of a freshly
+ * loaded table. What replaces it is {@value #READ_TIMEOUT_PROPERTY}, or the 0 this class has
+ * always put here where a deployment asks for nothing.
+ * <p>
+ * A read bound the connection string sets itself is neither replaced nor lifted: it was not set
+ * by us at the login either, so nothing of the administrator's is touched here.
+ * <p>
+ * Called whether or not the login had a bound to lift, since the standing bound is not the
+ * login's: with {@value #CONNECT_TIMEOUT_PROPERTY} at 0 the login is bounded by what is left of
+ * the deadline of the borrow instead, and a deployment running that way would otherwise set the
+ * property here and get nothing for it.
+ * <p>
+ * Returns whether the connection may be pooled. A connection still carrying the bound of its
+ * login must not be: it would fail the statements of every borrower after this one. A
+ * connection that merely never took the standing bound may be - that is the connection this
+ * pool handed out before the property existed.
+ */
+ private static boolean applyStandingReadBound(Connection con, String connectionString, ConnectDialect dialect,
+ long loginBoundSeconds, boolean readBoundSet) {
+ final int millis = standingReadBoundMillis(connectionString, dialect);
+ if (millis == 0 && !readBoundSet) {
+ return true; // nothing of ours on this connection: nothing to set here, and nothing to lift
+ }
+ final String consequence = readBoundSet
+ ? "statements taking longer than the " + loginBoundSeconds
+ + "s the login of this connection was bounded by fail on it, and it is closed rather than pooled"
+ : "this connection carries no read bound of its own, so a read of it the database stops answering waits"
+ + " with no deadline able to reach it";
+ return setNetworkTimeout(con, millis, consequence) || !readBoundSet;
}
/**
diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java
index 86a3fdf..328f7dd 100644
--- a/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java
+++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java
@@ -121,6 +121,120 @@
private JDBCBackendCfg config;
+ /** Not read yet: it follows {@link #poolKey()}, which a close and a re-open may leave naming another database. */
+ private static final int STANDING_READ_BOUND_UNREAD = -1;
+ private volatile int standingReadBound = STANDING_READ_BOUND_UNREAD;
+
+ /**
+ * The read bound this backend puts on its connections at their login, in milliseconds, or 0
+ * where it puts none - what {@link #applyBackstop} takes off a connection for the length of a
+ * statement that carries no bound of its own. Read once and remembered rather than per
+ * statement: it follows a system property and the connection string of the pool, and neither of
+ * them changes under a running statement.
+ * <p>
+ * Resolved against {@link #poolKey()} rather than against the configuration as it stands, for
+ * the reason {@link #getConnection(boolean)} borrows on that one: db-directory may be changed on
+ * a running backend, and the connections whose bound this decides are the ones of the pool
+ * {@link #open(AccessMode)} registered with. Read off the url the configuration names now, the
+ * lift would be decided for a pool this storage never borrows from - leaving the bound of this
+ * backend standing on a statement of an unbounded class, which is what {@code bulk.timeout=0}
+ * promises will not happen, or taking a bound of the deployment's own off the connections it
+ * really borrows.
+ * <p>
+ * Resolved once and for all in {@link #open(AccessMode)} rather than left to the first statement
+ * that asks: {@link #applyBackstop} is the only caller in production, and it asks only behind a
+ * statement of a class carrying no bound of its own - a deployment that gives {@code
+ * bulk.timeout} a value has no such statement anywhere, and would never be told that its two
+ * bounds are set the wrong way round.
+ */
+ int standingReadBoundMillis() {
+ int millis=standingReadBound;
+ if (millis < 0) {
+ millis=CachedConnection.standingReadBoundMillis(poolKey());
+ reportABoundNoStatementCanOutlive(millis);
+ standingReadBound=millis;
+ }
+ return millis;
+ }
+
+ /**
+ * Whether a standing read bound cuts a statement carrying a bound of its own short of it. Such
+ * a statement then dies on the socket - which costs the connection the driver closes, and names
+ * neither of the two properties that decided it - instead of being cancelled at the bound of its
+ * own class. A statement of a class with no bound at all is not weighed here: {@link
+ * #applyBackstop} takes the standing bound off for as long as one of those runs.
+ * <p>
+ * Weighed against {@link #backstopMillis} of that bound rather than against the bound itself,
+ * because the cancel is not always there to come first: the catalog lookups of {@code openTree()}
+ * ask {@code DatabaseMetaData}, which takes no query timeout at all, and any driver is free to
+ * refuse one. What ends such a statement is the socket layer of its own class, a margin later,
+ * and a standing bound anywhere below that ends it earlier - with {@link #applyBackstop} arming
+ * nothing on top of it, since the connection already carries the tighter of the two, so the
+ * failure arrives naming neither property.
+ */
+ static boolean cutsStatementsShort(int standingMillis, int statementSeconds) {
+ return standingMillis > 0 && statementSeconds > 0 && standingMillis <= backstopMillis(statementSeconds);
+ }
+
+ /** The loosest bound a statement of this backend carries, and the property that gives it. */
+ static final class LoosestBound {
+ final int seconds;
+ final String property;
+
+ LoosestBound(int seconds, String property) {
+ this.seconds = seconds;
+ this.property = property;
+ }
+ }
+
+ /**
+ * The loosest bound a statement of this backend may be given: what a standing read bound has to
+ * stand behind, since every one of those statements was told it may take that long.
+ * <p>
+ * Not {@link StatementBound#OPERATION} alone. The statistics refresh after an import has a
+ * property of its own, ten minutes by default, and legitimately takes as long as a scan of the
+ * table it describes; and a deployment that gives {@link StatementBound#BULK} a value takes that
+ * class out of the lift of {@link #applyBackstop} and into this weighing, since a bulk statement
+ * bounded by a property is a statement the standing bound can cut short like any other.
+ */
+ static LoosestBound loosestStatementBound() {
+ int seconds=statisticsTimeoutSeconds();
+ String property=STATISTICS_TIMEOUT_PROPERTY;
+ for (final StatementBound bound : StatementBound.values()) {
+ final int boundSeconds=bound.seconds();
+ if (boundSeconds > seconds) {
+ seconds=boundSeconds;
+ property=bound.property;
+ }
+ }
+ return new LoosestBound(seconds, property);
+ }
+
+ /**
+ * Says once that the two bounds were set the wrong way round. The socket read timeout is the
+ * layer behind the cancel of a statement, not in front of it: under the bound of the statement
+ * it is the one that fires, and what the operator then sees is a connection closed by its driver
+ * under a bare state of class 08 - {@link #timedOut} weighs the statement against its own bound,
+ * finds it well inside, and passes the failure through as it found it.
+ * <p>
+ * Said where the backend opens rather than where the bound is first needed, and weighed against
+ * {@link #loosestStatementBound()}: a deployment reaches this the moment it configures the two
+ * the wrong way round, whatever kind of statement it goes on to run.
+ */
+ private void reportABoundNoStatementCanOutlive(int millis) {
+ final LoosestBound loosest=loosestStatementBound();
+ if (cutsStatementsShort(millis, loosest.seconds) && standingReadBoundWarned.compareAndSet(false, true)) {
+ logger.warn(LocalizableMessage.raw("jdbc: the read bound of %s is %d ms, which a statement of this backend"
+ + " reaches before the %d s of %s it is given: such a statement is cut by the socket read timeout,"
+ + " closing the connection and naming neither property, rather than being cancelled at the bound of"
+ + " its own class. A standing read bound stands behind the bound of a statement - behind the %d s"
+ + " margin of that layer as well, since the cancel in front of it is one a driver may refuse and one"
+ + " the catalog lookups of a tree are never given - so it has to be the longer of the two",
+ CachedConnection.READ_TIMEOUT_PROPERTY, millis, loosest.seconds, loosest.property,
+ BACKSTOP_MARGIN_SECONDS));
+ }
+ }
+
public JDBCStorage(JDBCBackendCfg cfg, ServerContext serverContext) {
this.config = cfg;
cfg.addJDBCChangeListener(this);
@@ -138,6 +252,14 @@
try
{
this.config = cfg;
+ // The standing read bound is deliberately not reset here. It follows poolKey() - the
+ // connection string open() registered the pool with, not the one config names now - so a
+ // db-directory changed under a running backend does not move it. Reset, it would be
+ // resolved again under a lift already in flight: applyBackstop() would find the answer of
+ // another url for the connection whose read bound it has just taken off, fall through to
+ // giveBack() and hand that bound back to the statements of an unbounded class still
+ // running on it - the failure the lift exists to prevent, and one naming no property.
+ // What does move it is a close and a re-open, which is where it is reset (releasePool()).
}
catch (Exception e)
{
@@ -345,7 +467,7 @@
return 0; // no connection to arm it on: the cancel is the whole bound of such a statement
}
synchronized (state) {
- return state.armed;
+ return state.applied != null ? state.applied : 0; // a lift is a zero either way: nothing bounds it
}
}
@@ -432,6 +554,7 @@
private final AtomicBoolean backstopUnsupportedWarned = new AtomicBoolean();
private final AtomicBoolean backstopFailedWarned = new AtomicBoolean();
private final AtomicBoolean queryTimeoutWarned = new AtomicBoolean();
+ private final AtomicBoolean standingReadBoundWarned = new AtomicBoolean();
/**
* The socket read timeout of one connection, and the statements running on it. This second
@@ -456,10 +579,19 @@
int unbounded;
/** Statements holding this entry, bounded or not: at zero it leaves {@link #backstops}. */
int holders;
- /** What the connection carried before the backstop armed it, and is given back afterwards. */
+ /** What the connection carried before the backstop touched it, and is given back afterwards. */
int previous;
- /** What the backstop has armed, or 0 when the connection carries {@link #previous}. */
- int armed;
+ /**
+ * What this backstop has put on the connection: {@code null} where it has put nothing and the
+ * connection carries {@link #previous} of its own, 0 where the read bound is taken off for a
+ * statement carrying none, and the value armed otherwise.
+ * <p>
+ * One field rather than a value beside a flag, because "nothing of ours is on this
+ * connection" and "our lift is on it" are both a zero of that value: told apart by a boolean
+ * beside it, the pair has to be tested together at every site that gives the connection back,
+ * and an invariant spelled out at four sites is one three of them can be left out of.
+ */
+ Integer applied;
/**
* Set when the driver would not take a network timeout on this connection: it is not asked
* again while the statements holding this entry run. A connection is the right scope for
@@ -540,6 +672,11 @@
return (int) Math.min(Integer.MAX_VALUE, (seconds+BACKSTOP_MARGIN_SECONDS)*1000L);
}
+ /** Whether the read bound of this connection is the one this backstop took off for a statement carrying none. */
+ private static boolean lifted(Backstop state) {
+ return state.applied != null && state.applied == 0;
+ }
+
/**
* Makes the socket read timeout of the connection what the statements in flight on it need: the
* loosest of their bounds, or nothing of ours at all while one of them carries no bound. Called
@@ -559,29 +696,53 @@
final int wanted=state.unbounded > 0 || state.bounds.isEmpty() ? 0 : state.bounds.lastKey();
try {
if (wanted == 0) {
- if (state.armed != 0) {
- con.setNetworkTimeout(DIRECT_EXECUTOR, state.previous);
- state.armed=0;
+ // A statement of an unbounded class is running, and the connection carries the read
+ // bound this backend gave it at its login (CachedConnection.READ_TIMEOUT_PROPERTY):
+ // that bound comes off for as long as the statement does, since a statement told it
+ // may take as long as it needs must not be cut by a value armed for another one.
+ // Only ours is taken off - a read timeout standing in the connection string is the
+ // deployment's own, and lifting it would hand the connection back to the pool with
+ // the one bound its url asked for gone.
+ if (state.unbounded > 0 && standingReadBoundMillis() > 0) {
+ if (state.applied == null) {
+ state.previous=con.getNetworkTimeout();
+ }
+ if (state.previous > 0) {
+ if (!lifted(state)) { // whether this backstop had armed a value or put nothing on at all
+ con.setNetworkTimeout(DIRECT_EXECUTOR, 0);
+ state.applied=0;
+ }
+ return;
+ }
}
+ giveBack(con, state);
return;
}
- if (state.armed == 0) {
+ // What the connection carried is read once and remembered until it is given back. Read
+ // again while the lift above holds, it would be the 0 of that lift - and the read bound
+ // of the connection would go back to the pool gone for the rest of its life, which is
+ // how a statement of an unbounded class outliving a bounded one on the same connection
+ // takes the deployment's bound away for good.
+ if (state.applied == null) {
state.previous=con.getNetworkTimeout();
}
// only ever tighten: a connection that already carries a read timeout carries one a
- // deployment asked for, and this backstop exists to cap a cancel that is not acted
- // upon, not to relax anything. 0 is "no timeout" in the JDBC contract, so it is the
- // one value there is always something to gain by replacing.
+ // deployment asked for - the bound standing in its url, or the standing bound of
+ // CachedConnection.READ_TIMEOUT_PROPERTY this backend set at its login on their behalf -
+ // and this backstop exists to cap a cancel that is not acted upon, not to relax
+ // anything. The standing bound being ours to set makes it no less theirs to keep: it is
+ // sized to stand behind every statement of this backend, and one that does not is said
+ // where the backend opens (reportABoundNoStatementCanOutlive) rather than quietly worked
+ // around here, which would leave the property meaning something other than what it says.
+ // 0 is "no timeout" in the JDBC contract, so it is the one value there is always
+ // something to gain by replacing.
if (state.previous > 0 && state.previous <= wanted) {
- if (state.armed != 0) {
- con.setNetworkTimeout(DIRECT_EXECUTOR, state.previous);
- state.armed=0;
- }
+ giveBack(con, state);
return;
}
- if (state.armed != wanted) {
+ if (state.applied == null || state.applied != wanted) {
con.setNetworkTimeout(DIRECT_EXECUTOR, wanted);
- state.armed=wanted;
+ state.applied=wanted;
}
}catch (SQLException | RuntimeException e) {
state.failed=true; // whatever the cause, this connection is not asked again while it runs
@@ -612,13 +773,25 @@
}
/**
- * Gives the connection back the read timeout it carried before this backstop armed one, and
- * forgets having armed it. Best effort by construction: the caller reaches this from a driver
- * call that has just failed, so the connection may well be gone - and where it is, it is the
- * driver that closes it rather than this backend.
+ * Gives the connection back the read timeout it carried before this backstop touched it -
+ * whether that was a bound armed for a statement or the lift of one that carries none - and
+ * forgets having touched it. Nothing to do for a connection this backstop left alone.
+ */
+ private static void giveBack(Connection con, Backstop state) throws SQLException {
+ if (state.applied == null) {
+ return; // the connection carries its own value already
+ }
+ con.setNetworkTimeout(DIRECT_EXECUTOR, state.previous);
+ state.applied=null;
+ }
+
+ /**
+ * The same, best effort by construction: the caller reaches this from a driver call that has
+ * just failed, so the connection may well be gone - and where it is, it is the driver that
+ * closes it rather than this backend.
*/
private static void restorePrevious(Connection con, Backstop state) {
- if (state.armed == 0) {
+ if (state.applied == null) {
return; // the connection carries its own value already
}
try {
@@ -627,7 +800,7 @@
// nothing further can be done for this connection here, and the failure to report is the
// one that brought us into the catch above
}finally {
- state.armed=0;
+ state.applied=null;
}
}
@@ -771,6 +944,14 @@
CachedConnection.openPool(poolConnectionString);
registeredHere=true;
}
+ // Resolved here, against the connection string the pool was just registered with, rather
+ // than left to the first statement that needs it: applyBackstop() is the only caller in
+ // production and reaches it only behind a statement of a class carrying no bound of its
+ // own, so a deployment that gives bulk.timeout a value of its own reaches it nowhere at
+ // all - and the word reportABoundNoStatementCanOutlive() owes an operator whose two
+ // bounds are set the wrong way round would never be said. Costs one system property and
+ // one scan of the url, once per open.
+ standingReadBoundMillis();
// The validated borrow is the whole of the open, and nothing is taken from it here: the
// status is set below rather than inside the block, or a throw from the implicit close()
// - the rollback of the return goes to the database - would leave the storage reporting
@@ -793,6 +974,7 @@
// touching the pool: left standing it would send the close() of this storage to
// releasePool() for a registration it never made.
poolConnectionString=null;
+ standingReadBound=STANDING_READ_BOUND_UNREAD; // it follows poolKey(), which the next open may register elsewhere
poolRegistered.set(false);
}
throw e;
@@ -806,6 +988,7 @@
if (poolRegistered.compareAndSet(true, false)) {
final String registered=poolConnectionString;
poolConnectionString=null;
+ standingReadBound=STANDING_READ_BOUND_UNREAD; // it follows poolKey(), which a re-open may register elsewhere
if (registered!=null) {
CachedConnection.closePool(registered);
}
@@ -1888,6 +2071,17 @@
static final String STATISTICS_TIMEOUT_PROPERTY=STATISTICS_PROPERTY+".timeout";
private static final int STATISTICS_TIMEOUT_SECONDS_DEFAULT=600;
+ /**
+ * What the statistics refresh may take, as configured. Read where the refresh runs and again
+ * where a standing read bound is weighed against the statements of this backend
+ * ({@link #loosestStatementBound()}): it is the loosest bound any statement here is given by
+ * default, so a standing bound under it cuts the refresh short of the very property that was
+ * meant to bound it.
+ */
+ static int statisticsTimeoutSeconds() {
+ return clampSeconds(Integer.getInteger(STATISTICS_TIMEOUT_PROPERTY,STATISTICS_TIMEOUT_SECONDS_DEFAULT));
+ }
+
// A bulk load leaves the optimizer statistics of freshly created tables stale (a table that
// was never analyzed can make the planner badly misestimate the "where k>? order by k" cursor
// batches - see OpenIdentityPlatform/OpenDJ#859), so refresh them once the data is in place.
@@ -1904,7 +2098,7 @@
if (dialect==null) { // no portable statistics refresh for other engines
return false; // nothing was refreshed: reporting success here would make the assertion of the tests vacuous
}
- final int timeoutSeconds=clampSeconds(Integer.getInteger(STATISTICS_TIMEOUT_PROPERTY,STATISTICS_TIMEOUT_SECONDS_DEFAULT));
+ final int timeoutSeconds=statisticsTimeoutSeconds();
boolean allRefreshed=true;
for (final TreeName treeName : trees) {
final String tableName=getTableName(treeName);
diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CachedConnectionTestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CachedConnectionTestCase.java
index 8524d4b..3e834f3 100644
--- a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CachedConnectionTestCase.java
+++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CachedConnectionTestCase.java
@@ -104,6 +104,9 @@
/** The window as this JVM was started with it, put back after every test that varies it. */
private static final long CONFIGURED_ALIVE_BYPASS_NANOS = CachedConnection.aliveBypassNanos;
+ /** The standing read bound as this JVM was started with it, put back after every test that varies it. */
+ private static final int CONFIGURED_READ_TIMEOUT_MILLIS = CachedConnection.readTimeoutMillis;
+
@BeforeClass
public void registerStubDriver() throws Exception {
DriverManager.registerDriver(stub);
@@ -131,10 +134,12 @@
System.clearProperty(CachedConnection.POOL_MAX_PROPERTY);
System.clearProperty(CachedConnection.TTL_PROPERTY);
System.clearProperty(CachedConnection.ALIVE_BYPASS_PROPERTY);
+ System.clearProperty(CachedConnection.READ_TIMEOUT_PROPERTY);
// what has been reported once is remembered for the life of the jvm: left standing, the key
// of one test is what the next one finds when it asserts that it reported something itself
CachedConnection.warnedOnce.clear();
CachedConnection.aliveBypassNanos = CONFIGURED_ALIVE_BYPASS_NANOS;
+ CachedConnection.readTimeoutMillis = CONFIGURED_READ_TIMEOUT_MILLIS;
}
/**
@@ -1999,6 +2004,259 @@
}
/**
+ * What a connection carries once the login is through, where a deployment asked for a read
+ * bound of its own: that bound rather than the bound of the login, which is a value nothing
+ * slower than a connect is meant to be measured against. Without one, this is the lift above -
+ * the behaviour of every connection this backend established before the property existed.
+ */
+ @Test(timeOut = 120000)
+ public void testAnEstablishedConnectionCarriesTheReadBoundAskedFor() throws Exception {
+ final String url = StubDriver.PREFIX + "standing-read-bound";
+ CachedConnection.readTimeoutMillis = 90000;
+ final Connection parent = mock(Connection.class);
+ stub.answerWith(parent);
+
+ CachedConnection.connect(url, CachedConnection.ConnectDialect.MYSQL, 30,
+ CachedConnection.poolOf(url), false);
+
+ verify(parent).setNetworkTimeout(any(Executor.class), eq(90000));
+ verify(parent, never()).setNetworkTimeout(any(Executor.class), eq(0));
+ }
+
+ /**
+ * And it carries it whether or not the login had a bound of its own to lift. The read bound of
+ * a login is only ever set where the connect is bounded, so a deployment that runs with
+ * {@code connect.timeout=0} - the one setting that leaves a connect to the deadline of the
+ * borrow alone - would otherwise set this property and get nothing for it.
+ */
+ @Test(timeOut = 120000)
+ public void testTheReadBoundIsSetWhereTheLoginHadNoneToLift() throws Exception {
+ final String url = StubDriver.PREFIX + "read-bound-without-a-login-bound";
+ CachedConnection.readTimeoutMillis = 90000;
+ final Connection parent = mock(Connection.class);
+ stub.answerWith(parent);
+
+ CachedConnection.connect(url, CachedConnection.ConnectDialect.MYSQL, 0,
+ CachedConnection.poolOf(url), false);
+
+ verify(parent).setNetworkTimeout(any(Executor.class), eq(90000));
+ }
+
+ /**
+ * A read bound standing in the connection string is the deployment's own: the connect does not
+ * replace it with this one, exactly as it does not set the read bound of a login on top of it.
+ */
+ @Test(timeOut = 120000)
+ public void testAReadBoundOfTheUrlIsNotReplacedByTheConfiguredOne() throws Exception {
+ final String url = StubDriver.PREFIX + "own-read-bound?socketTimeout=1000";
+ CachedConnection.readTimeoutMillis = 90000;
+ final Connection parent = mock(Connection.class);
+ stub.answerWith(parent);
+
+ CachedConnection.connect(url, CachedConnection.ConnectDialect.MYSQL, 30,
+ CachedConnection.poolOf(url), false);
+
+ verify(parent, never()).setNetworkTimeout(any(Executor.class), anyInt());
+ }
+
+ /**
+ * Which connections carry a read bound of this backend's own making, as the backstop of
+ * {@code JDBCStorage} has to know it: a statement of an unbounded class takes that bound off
+ * for as long as it runs, and it may only take off what this class put on. A bound of the url
+ * is the deployment's, and a driver whose property names are not known here was never given
+ * one - lifting either would leave the connection unbounded for the rest of its life.
+ */
+ @Test(timeOut = 120000)
+ public void testOnlyTheReadBoundThisClassSetsIsItsOwnToLift() {
+ CachedConnection.readTimeoutMillis = 90000;
+ assertEquals(CachedConnection.standingReadBoundMillis("jdbc:mysql://localhost:3306/db"), 90000,
+ "the bound this class sets on a connection of a dialect it knows");
+ assertEquals(CachedConnection.standingReadBoundMillis("jdbc:mysql://localhost:3306/db?socketTimeout=1000"), 0,
+ "a read bound of the url was reported as this backend's own");
+ assertEquals(CachedConnection.standingReadBoundMillis("jdbc:h2:mem:db"), 0,
+ "a driver this class sets no read bound on was reported as bounded by it");
+
+ CachedConnection.readTimeoutMillis = 0;
+ assertEquals(CachedConnection.standingReadBoundMillis("jdbc:mysql://localhost:3306/db"), 0,
+ "a connection carries no standing bound where none is configured");
+ }
+
+ /**
+ * The bound is configured in seconds and reaches the driver in milliseconds; 0, a negative
+ * value and a value that is no number all leave a connection unbounded, which is what this
+ * backend did before the property existed. A value past the ceiling of a socket read timeout is
+ * taken down to it rather than left to overflow the {@code int} of setNetworkTimeout, where it
+ * would arrive as a negative timeout - a value outside the contract, and one a driver is free
+ * to read as anything at all.
+ */
+ @Test(timeOut = 120000)
+ public void testTheReadBoundIsConfiguredInSeconds() {
+ assertEquals(CachedConnection.getReadTimeoutMillis(), 0, "a connection is unbounded by default");
+
+ System.setProperty(CachedConnection.READ_TIMEOUT_PROPERTY, "90");
+ assertEquals(CachedConnection.getReadTimeoutMillis(), 90000);
+
+ System.setProperty(CachedConnection.READ_TIMEOUT_PROPERTY, "0");
+ assertEquals(CachedConnection.getReadTimeoutMillis(), 0);
+
+ System.setProperty(CachedConnection.READ_TIMEOUT_PROPERTY, "-1");
+ assertEquals(CachedConnection.getReadTimeoutMillis(), 0, "a negative value was not read as no bound");
+
+ System.setProperty(CachedConnection.READ_TIMEOUT_PROPERTY, "a minute and a half");
+ assertEquals(CachedConnection.getReadTimeoutMillis(), 0,
+ "a value that is no number was not ignored in favour of the default");
+
+ System.setProperty(CachedConnection.READ_TIMEOUT_PROPERTY, Integer.toString(Integer.MAX_VALUE));
+ assertEquals(CachedConnection.getReadTimeoutMillis(), JDBCStorage.MAX_BOUND_SECONDS * 1000,
+ "a value past the ceiling of a socket read timeout was not taken down to it");
+ }
+
+ /**
+ * A driver that will not take the standing bound leaves a connection with no bound of ours on
+ * it, which is what every connection of this pool carried before the property existed and no
+ * reason to keep this one out of the pool. The connection that must not be pooled is the one
+ * still carrying the read bound of its login: there the call that failed was a call to take
+ * something off, and the bound left on it fails every statement slower than a connect.
+ */
+ @Test(timeOut = 120000)
+ public void testAConnectionThatWouldNotTakeTheStandingBoundIsStillPooled() throws Exception {
+ final String url = StubDriver.PREFIX + "unsettable-standing-bound";
+ CachedConnection.readTimeoutMillis = 90000;
+ final Connection parent = mock(Connection.class);
+ doThrow(new SQLException("setNetworkTimeout is not supported"))
+ .when(parent).setNetworkTimeout(any(Executor.class), anyInt());
+ stub.answerWith(parent);
+
+ final CachedConnection.Pool pool = CachedConnection.poolOf(url);
+ // Metered, and holding a permit of the pool as a borrow does: an unmetered connection is
+ // closed rather than pooled whatever bound it carries, which would answer this on the
+ // accounting of the pool instead of on the bound the case is about.
+ assertTrue(pool.tryReserve(), "the pool of this url would not reserve a place for the connection");
+ final CachedConnection borrowed = CachedConnection.connect(url, CachedConnection.ConnectDialect.MYSQL, 0,
+ pool, true);
+ borrowed.close();
+
+ verify(parent, never()).close();
+ assertEquals(pool.idleCount(), 1,
+ "a connection carrying no bound of ours was kept out of the pool");
+ }
+
+ /**
+ * A read parameter of the url set to 0 is no bound of the deployment's: 0 is what every one of
+ * these drivers reads as "wait as long as it takes", which is the default this property exists
+ * to replace. It tells the two bounds apart, and only on postgresql, where a parameter of the
+ * url outranks the property this class supplies: the login there is left carrying no bound of
+ * ours, and rightly so, while the bound of this property is no property of a connect at all - it
+ * is a setNetworkTimeout of an established connection, which no url outranks. Read as a bound of
+ * theirs, a "socketTimeout=0" - the default of pgjdbc, written out - would leave a deployment
+ * that asked for this one with no bound and no report of why.
+ */
+ @Test(timeOut = 120000)
+ public void testAReadParameterOfTheUrlSetToZeroIsNoBoundOfTheDeployments() {
+ CachedConnection.readTimeoutMillis = 90000;
+ assertEquals(CachedConnection.standingReadBoundMillis("jdbc:postgresql://localhost/db?socketTimeout=0"), 90000,
+ "a postgresql url turning the read bound off was read as a bound of the deployment's own");
+ assertEquals(CachedConnection.standingReadBoundMillis("jdbc:mysql://localhost:3306/db?socketTimeout=0"), 90000,
+ "a mysql url turning the read bound off was read as a bound of the deployment's own");
+ assertEquals(CachedConnection.standingReadBoundMillis("jdbc:postgresql://localhost/db?socketTimeout=30"), 0,
+ "a read bound of a postgresql url is the deployment's own and stands");
+ }
+
+ /**
+ * The predicate deciding whether a url bounds the read reads the same set of names as the one
+ * deciding whether it declares it. It used to stop at the first name present even where the
+ * value there was a zero, so a url naming the bound under both names of the oracle driver - the
+ * dotted one turned off, the last segment set - was declared() and not bounds(): the login kept
+ * the administrator's value, because a property of ours is not supplied over a declared one, and
+ * a bound of ours then went on top of it with setNetworkTimeout. Contrived, but "the bound taken
+ * off is the bound that was set" holds only while the two look at the same names.
+ */
+ @Test(timeOut = 120000)
+ public void testAReadBoundUnderEitherNameOfTheUrlIsTheDeploymentsOwn() {
+ CachedConnection.readTimeoutMillis = 90000;
+ assertEquals(CachedConnection.standingReadBoundMillis(
+ "jdbc:oracle:thin:@//localhost:1521/db?oracle.jdbc.ReadTimeout=0&ReadTimeout=600"), 0,
+ "a bound standing under the last segment of the name was read as no bound at all");
+ assertEquals(CachedConnection.standingReadBoundMillis(
+ "jdbc:oracle:thin:@//localhost:1521/db?oracle.jdbc.ReadTimeout=0&ReadTimeout=0"), 90000,
+ "a url turning the read bound off under both of its names is no bound of the deployment's");
+ }
+
+ /**
+ * With nothing configured this is the lift and nothing else - the read bound of the login comes
+ * off and no bound of ours goes on top of it, which is what every connection of this pool
+ * carried before the property existed. The default of this property is what makes the change
+ * that introduced it no change at all for a deployment that does not ask for one.
+ */
+ @Test(timeOut = 120000)
+ public void testTheDefaultTakesTheBoundOfTheLoginOffAndPutsNothingOnTopOfIt() throws Exception {
+ final String url = StubDriver.PREFIX + "default-read-bound";
+ CachedConnection.readTimeoutMillis = 0;
+ final Connection parent = mock(Connection.class);
+ stub.answerWith(parent);
+
+ CachedConnection.connect(url, CachedConnection.ConnectDialect.MYSQL, 30,
+ CachedConnection.poolOf(url), false);
+
+ verify(parent).setNetworkTimeout(any(Executor.class), eq(0));
+ verify(parent, times(1)).setNetworkTimeout(any(Executor.class), anyInt());
+ }
+
+ /**
+ * A driver that would take neither the standing bound nor the lift leaves the connection that
+ * must not be pooled: what it is left carrying is the read bound of a connect, and every borrow
+ * after this one would meet it - which is the case above, reached by the other of the two paths
+ * that call for a setNetworkTimeout once the login is through.
+ */
+ @Test(timeOut = 120000)
+ public void testAConnectionThatWouldTakeNeitherTheStandingBoundNorTheLiftIsNotPooled() throws Exception {
+ final String url = StubDriver.PREFIX + "unsettable-over-a-login-bound";
+ CachedConnection.readTimeoutMillis = 90000;
+ final Connection parent = mock(Connection.class);
+ doThrow(new SQLException("setNetworkTimeout is not supported"))
+ .when(parent).setNetworkTimeout(any(Executor.class), anyInt());
+ stub.answerWith(parent);
+
+ final CachedConnection.Pool pool = CachedConnection.poolOf(url);
+ // Metered, and holding a permit of the pool as a borrow does: an unmetered connection is
+ // closed rather than pooled whatever bound it carries, which would answer this on the
+ // accounting of the pool instead of on the bound the case is about.
+ assertTrue(pool.tryReserve(), "the pool of this url would not reserve a place for the connection");
+ final CachedConnection borrowed = CachedConnection.connect(url, CachedConnection.ConnectDialect.MYSQL, 30,
+ pool, true);
+ borrowed.close();
+
+ verify(parent).close();
+ assertEquals(pool.idleCount(), 0,
+ "a connection still carrying the read bound of its login went back into the pool");
+ }
+
+ /**
+ * The initializer reads this property the way it reads the two windows above - a value that is
+ * no number, or a negative one, is reported once and ignored in favour of the default - and
+ * reporting it has to leave the class usable: the set that report is deduplicated through is
+ * declared above every field whose initializer can reach it (JLS 12.4.2), so a field of this
+ * one moved above that set would turn a typo in a property into an ExceptionInInitializerError
+ * that no test of a class already initialized would ever meet.
+ */
+ @Test(timeOut = 120000, dataProvider = "readBoundsWorthWarningAbout")
+ public void testAReadBoundWorthWarningAboutStillInitializesTheClass(String configured) throws Exception {
+ System.setProperty(CachedConnection.READ_TIMEOUT_PROPERTY, configured);
+
+ final Class<?> reloaded = loadedAfresh(CachedConnection.class);
+
+ assertNotSame(reloaded, CachedConnection.class, "the class under test was not loaded afresh");
+ final Field field = reloaded.getDeclaredField("readTimeoutMillis");
+ field.setAccessible(true);
+ assertEquals(field.getInt(null), 0, "the bound the reloaded class settled on");
+ }
+
+ @DataProvider
+ public Object[][] readBoundsWorthWarningAbout() {
+ return new Object[][]{{"a minute and a half"}, {"-1"}};
+ }
+
+ /**
* The case the window exists for: the connection this borrow takes out answered the database a
* moment ago, and asking it again costs the round trip the operation came to make.
*/
diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStatementBoundTestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStatementBoundTestCase.java
index 32ad5a5..0430507 100644
--- a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStatementBoundTestCase.java
+++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStatementBoundTestCase.java
@@ -52,6 +52,7 @@
import static org.mockito.Mockito.anyInt;
import static org.mockito.Mockito.anyString;
import static org.mockito.Mockito.atLeastOnce;
+import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.eq;
import static org.mockito.Mockito.inOrder;
@@ -100,9 +101,42 @@
System.clearProperty(bound.property);
}
System.clearProperty(JDBCStorage.STATISTICS_TIMEOUT_PROPERTY);
+ System.clearProperty(CachedConnection.READ_TIMEOUT_PROPERTY);
+ // a static of the pool rather than a property of this storage: left standing, the bound one
+ // test puts on its connections is the bound every test after it finds on them
+ CachedConnection.readTimeoutMillis = CONFIGURED_READ_TIMEOUT_MILLIS;
storage.accessMode = AccessMode.READ_ONLY; // an import test opens it for writing
}
+ /** The standing read bound as this JVM was started with it, put back after every test that varies it. */
+ private static final int CONFIGURED_READ_TIMEOUT_MILLIS = CachedConnection.readTimeoutMillis;
+
+ /**
+ * A connection that keeps the read timeout it is given, the way a driver does. A mock answering
+ * a fixed {@code getNetworkTimeout()} cannot tell the two apart: a backstop that reads what the
+ * connection carried once and keeps it, and one that reads it again after having changed the
+ * value itself - which is how the standing bound of a connection is lost for the rest of its
+ * life in the pool.
+ */
+ private static Connection connectionCarrying(int readTimeoutMillis) throws SQLException {
+ final Connection con = mock(Connection.class);
+ final AtomicInteger carried = new AtomicInteger(readTimeoutMillis);
+ when(con.getNetworkTimeout()).thenAnswer(new Answer<Integer>() {
+ @Override
+ public Integer answer(InvocationOnMock invocation) {
+ return carried.get();
+ }
+ });
+ doAnswer(new Answer<Void>() {
+ @Override
+ public Void answer(InvocationOnMock invocation) {
+ carried.set((Integer) invocation.getArguments()[1]);
+ return null;
+ }
+ }).when(con).setNetworkTimeout(any(Executor.class), anyInt());
+ return con;
+ }
+
/** How long a test waits for a statement running on another thread before it fails. */
private static final long WAIT_MILLIS = 30000;
@@ -372,6 +406,214 @@
}
/**
+ * The other half of that, for the read bound a connection of this pool carries all its life:
+ * a statement of an unbounded class takes it off for as long as it runs. The socket read
+ * timeout of {@code CachedConnection.READ_TIMEOUT_PROPERTY} is armed at the login and never
+ * disarmed, so a count of a populated table or the delete that empties a tree before an import
+ * would die at it - and die naming no property at all, since a statement of an unbounded class
+ * has none in force to name.
+ */
+ @Test
+ public void testABulkStatementTakesTheStandingReadBoundOffTheConnection() throws Exception {
+ CachedConnection.readTimeoutMillis = 90000; // as the login of this connection put it on
+ final Connection con = connectionCarrying(90000);
+ final PreparedStatement bulk = mock(PreparedStatement.class);
+ when(bulk.getConnection()).thenReturn(con);
+ when(bulk.executeUpdate()).thenReturn(1);
+
+ storage.execute(bulk, StatementBound.BULK);
+
+ final InOrder inOrder = inOrder(con);
+ inOrder.verify(con).setNetworkTimeout(any(Executor.class), eq(0));
+ inOrder.verify(con).setNetworkTimeout(any(Executor.class), eq(90000));
+ assertEquals(con.getNetworkTimeout(), 90000, "the connection was left without the bound it came with");
+ }
+
+ /**
+ * Only the bound this backend set is this backend's to take off. A read timeout standing in the
+ * connection string is the deployment's own - the connect leaves it alone rather than replacing
+ * it - and lifting it for a bulk statement would hand the connection back to the pool with the
+ * one bound its url asked for gone.
+ */
+ @Test
+ public void testAReadBoundOfTheConnectionStringIsNotTakenOff() throws Exception {
+ CachedConnection.readTimeoutMillis = 90000;
+ final JDBCBackendCfg cfg = mockCfg(JDBCBackendCfg.class);
+ when(cfg.getDBDirectory()).thenReturn("jdbc:postgresql://localhost/test?socketTimeout=600");
+ final JDBCStorage bounded = new JDBCStorage(cfg, null);
+ final Connection con = connectionCarrying(600000);
+ final PreparedStatement bulk = mock(PreparedStatement.class);
+ when(bulk.getConnection()).thenReturn(con);
+ when(bulk.executeUpdate()).thenReturn(1);
+
+ bounded.execute(bulk, StatementBound.BULK);
+
+ verify(con, never()).setNetworkTimeout(any(Executor.class), anyInt());
+ }
+
+ /**
+ * A standing read bound at or under the bound of an ordinary statement is worth a word: the
+ * statement dies on the socket at it instead of being cancelled at the bound of its own class -
+ * which costs the connection the driver closes, and reports neither of the two properties that
+ * decided it. Above that bound the two compose, the cancel of the statement coming first and
+ * the standing bound staying behind it as the backstop of a cancel that is not acted upon. A
+ * class carrying no bound of its own is not cut by this at all: the bound comes off for as long
+ * as such a statement runs.
+ */
+ @Test(timeOut = 120000)
+ public void testAStandingReadBoundUnderTheBoundOfAStatementCutsItShort() {
+ final int backstop = (120 + JDBCStorage.BACKSTOP_MARGIN_SECONDS) * 1000;
+ assertTrue(JDBCStorage.cutsStatementsShort(60000, 120), "a bound under the bound of the statement");
+ assertTrue(JDBCStorage.cutsStatementsShort(120000, 120), "a bound the statement reaches at the same moment");
+ // Weighed against the socket layer of that bound, not against its cancel: the catalog lookups
+ // of openTree() are given no cancel at all, so what ends them is the layer a margin later -
+ // and a standing bound anywhere below that ends them earlier, with the backstop arming
+ // nothing on top of it because the connection already carries the tighter of the two.
+ assertTrue(JDBCStorage.cutsStatementsShort(140000, 120),
+ "a bound between the cancel of the statement and the socket layer behind it");
+ assertTrue(JDBCStorage.cutsStatementsShort(backstop, 120), "a bound that layer reaches at the same moment");
+ assertFalse(JDBCStorage.cutsStatementsShort(backstop + 1, 120), "a bound both layers come before");
+ assertFalse(JDBCStorage.cutsStatementsShort(0, 120), "no standing bound at all");
+ assertFalse(JDBCStorage.cutsStatementsShort(60000, 0), "a statement of an unbounded class, which is lifted");
+ }
+
+ /**
+ * What a standing read bound has to stand behind is the loosest bound a statement of this
+ * backend carries, not the bound of an ordinary one. The statistics refresh after an import has
+ * a property of its own - ten minutes by default, and it legitimately takes as long as a scan of
+ * the table it describes - so a standing bound of five cuts it on the socket, closing the
+ * importer's connection under a bare class-08 state naming neither property, and the statistics
+ * of #859 are then never refreshed. A bulk.timeout a deployment sets is in the same place: the
+ * class is no longer lifted, so its bound is weighed like any other.
+ */
+ @Test(timeOut = 120000)
+ public void testTheLoosestBoundOfAStatementIsWhatAStandingBoundHasToOutlive() {
+ assertEquals(JDBCStorage.loosestStatementBound().property, JDBCStorage.STATISTICS_TIMEOUT_PROPERTY,
+ "the statistics refresh is the loosest bound this backend gives a statement by default");
+ assertEquals(JDBCStorage.loosestStatementBound().seconds, 600);
+ assertTrue(JDBCStorage.cutsStatementsShort(300000, JDBCStorage.loosestStatementBound().seconds),
+ "a standing bound of five minutes was not weighed against the ten of the statistics refresh");
+
+ System.setProperty(JDBCStorage.STATISTICS_TIMEOUT_PROPERTY, "0"); // the refresh left unbounded
+ assertEquals(JDBCStorage.loosestStatementBound().property, StatementBound.OPERATION.property);
+ assertEquals(JDBCStorage.loosestStatementBound().seconds, 120);
+
+ System.setProperty(StatementBound.BULK.property, "3600"); // a class the lift no longer covers
+ assertEquals(JDBCStorage.loosestStatementBound().property, StatementBound.BULK.property);
+ assertEquals(JDBCStorage.loosestStatementBound().seconds, 3600);
+ }
+
+ /**
+ * The bound follows the connection string the pool was registered with, the way every other path
+ * that names a pool does. db-directory may be changed on a running backend and the borrow still
+ * leaves the pool open() registered with, so a bound resolved against the url config names now
+ * would be the answer for a pool this storage never borrows from: a bulk statement of the
+ * registered one would find the lift gated off and die at a bound bulk.timeout=0 promises it will
+ * not meet, and the reverse pairing would lift a bound that is the deployment's own.
+ * <p>
+ * It is not resolved again after the change either. Read again while a lift is in flight, the
+ * answer of another url would send applyBackstop() to giveBack() and re-arm the bound under the
+ * statements the lift took it off for - both of them dying at it, and neither naming a property.
+ */
+ @Test(timeOut = 120000)
+ public void testTheStandingReadBoundFollowsTheUrlThePoolWasRegisteredWith() throws Exception {
+ CachedConnection.readTimeoutMillis = 90000;
+ final JDBCBackendCfg cfg = mockCfg(JDBCBackendCfg.class);
+ when(cfg.getDBDirectory()).thenReturn("jdbc:mysql://registered/db");
+ final JDBCStorage registered = openedOn(cfg);
+ try {
+ assertEquals(registered.standingReadBoundMillis(), 90000, "the bound of the url it registered with");
+
+ // the configuration changed under the running backend, to a url whose own read bound is
+ // the deployment's: the borrow still leaves the pool of the url above
+ when(cfg.getDBDirectory()).thenReturn("jdbc:mysql://changed/db?socketTimeout=600");
+ registered.applyConfigurationChange(cfg);
+
+ assertEquals(registered.standingReadBoundMillis(), 90000,
+ "the lift was decided against a pool this storage does not borrow from");
+ } finally {
+ registered.close();
+ }
+ }
+
+ /**
+ * And it is resolved while the backend opens, not at the first statement that needs it.
+ * applyBackstop() is the only place production asks, and it asks only behind a statement of a
+ * class carrying no bound of its own - a deployment that gives bulk.timeout a value of its own
+ * has no such statement anywhere, so the word owed to an operator whose two bounds are set the
+ * wrong way round would never be said at all.
+ */
+ @Test(timeOut = 120000)
+ public void testTheStandingReadBoundIsResolvedWhileTheBackendOpens() throws Exception {
+ System.setProperty(StatementBound.BULK.property, "3600"); // no statement of an unbounded class anywhere
+ CachedConnection.readTimeoutMillis = 90000;
+ final JDBCBackendCfg cfg = mockCfg(JDBCBackendCfg.class);
+ when(cfg.getDBDirectory()).thenReturn("jdbc:mysql://resolved-at-open/db");
+ final JDBCStorage opened = openedOn(cfg);
+ try {
+ // what the answer would be if it were resolved now, on the first statement to ask
+ CachedConnection.readTimeoutMillis = 37000;
+
+ assertEquals(opened.standingReadBoundMillis(), 90000,
+ "the bound was not resolved while the backend opened");
+ } finally {
+ opened.close();
+ }
+ }
+
+ /**
+ * A storage opened on a configuration, borrowing nothing from a database: open() registers the
+ * pool of the url - which costs no connect - and the validating borrow of the open is answered
+ * with a mock, so what is left is the registration this suite is about.
+ */
+ private static JDBCStorage openedOn(JDBCBackendCfg cfg) throws Exception {
+ final Connection con = mock(Connection.class);
+ final JDBCStorage opening = new JDBCStorage(cfg, null) {
+ @Override
+ Connection getConnection(boolean trusted) {
+ return con;
+ }
+ };
+ opening.open(AccessMode.READ_WRITE);
+ return opening;
+ }
+
+ /**
+ * What the connection carried before is remembered across the lift, not read back off the
+ * connection while it is lifted: a bounded statement that outlives the bulk one takes the
+ * backstop of its own class, and the standing bound - not the zero of the lift - is what goes
+ * back when the last of them is through. Read again mid-flight, it would be the zero, and the
+ * connection would go back to the pool with no read bound at all for the rest of its life.
+ */
+ @Test
+ public void testTheStandingReadBoundOutlivesTheLiftAndComesBackAfterIt() throws Exception {
+ System.setProperty(StatementBound.OPERATION.property, "7");
+ CachedConnection.readTimeoutMillis = 90000;
+ final Connection con = connectionCarrying(90000);
+ final CountDownLatch bulkRunning = new CountDownLatch(1);
+ final CountDownLatch bulkMayFinish = new CountDownLatch(1);
+ final CountDownLatch operationRunning = new CountDownLatch(1);
+ final CountDownLatch operationMayFinish = new CountDownLatch(1);
+ final PreparedStatement bulk = lingering(con, bulkRunning, bulkMayFinish);
+ final PreparedStatement operation = lingering(con, operationRunning, operationMayFinish);
+
+ final Background clearTree = start("clear-tree", () -> storage.execute(bulk, StatementBound.BULK));
+ awaitOrFail(bulkRunning, "the bulk statement never started");
+ final Background entryRead = start("entry-read", () -> storage.execute(operation));
+ awaitOrFail(operationRunning, "the entry read never started");
+ bulkMayFinish.countDown();
+ clearTree.joinOrFail();
+ operationMayFinish.countDown();
+ entryRead.joinOrFail();
+
+ final InOrder inOrder = inOrder(con);
+ inOrder.verify(con).setNetworkTimeout(any(Executor.class), eq(0)); // the bulk statement takes it off
+ inOrder.verify(con).setNetworkTimeout(any(Executor.class), eq((7 + JDBCStorage.BACKSTOP_MARGIN_SECONDS) * 1000));
+ inOrder.verify(con).setNetworkTimeout(any(Executor.class), eq(90000));
+ assertEquals(con.getNetworkTimeout(), 90000, "the connection was left without the bound it came with");
+ }
+
+ /**
* The backstop belongs to the connection, not to the statement that armed it: the first
* statement to finish must not take it away from the statements still running there.
*/
diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java
index 7e4241b..0272ec0 100644
--- a/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java
+++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java
@@ -240,6 +240,33 @@
}
}
+ /**
+ * And the bound that replaces it reaches the socket of the driver: set with setNetworkTimeout
+ * once the login is through, it is read back off the connection the pool hands out (#885). The
+ * unit tests pin which value is set, on a mock that can only answer that it was asked; this is
+ * the driver of a real engine answering that it took it.
+ */
+ @Test(timeOut = 120000)
+ public void testTheStandingReadBoundReachesTheSocket() throws Exception {
+ final String url = createBackendCfg().getDBDirectory();
+ final int configured = CachedConnection.readTimeoutMillis;
+ CachedConnection.readTimeoutMillis = 5000;
+ try {
+ assertEquals(CachedConnection.standingReadBoundMillis(url), 5000,
+ "the url of this container carries a read bound of its own, so no bound of ours is set on it");
+ // a pooled connection would be handed back without being established again
+ CachedConnection.poolOf(url).drainIdle();
+ try (final Connection con = CachedConnection.getConnection(url)) {
+ assertEquals(con.getNetworkTimeout(), 5000,
+ "the read bound of this backend did not reach the socket of this driver");
+ }
+ } finally {
+ CachedConnection.readTimeoutMillis = configured;
+ // and nothing carrying the bound of this test goes back to the pool the suite goes on using
+ CachedConnection.poolOf(url).drainIdle();
+ }
+ }
+
private static ByteString key(int i) {
return ByteString.valueOfUtf8(String.format("key%02d", i));
}
--
Gitblit v1.10.0