+ * 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. + *
+ * 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. + *
+ * 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. + *
+ * 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. + *
+ * 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; @@ -878,6 +970,46 @@ private static boolean contains(int[] codes, int code) { 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. + *
+ * 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 @@ -1188,6 +1320,13 @@ static Connection getConnection(String connectionString, boolean trusted) throws * 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. + *
+ * 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) { @@ -1197,11 +1336,19 @@ private static void reportUnknownDialect(String connectionString, ConnectDialect 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); } /** @@ -1392,12 +1539,10 @@ static CachedConnection connect(String connectionString, ConnectDialect dialect, // 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; @@ -1407,19 +1552,41 @@ static CachedConnection connect(String connectionString, ConnectDialect dialect, 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. + *
+ * 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. + *
+ * 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. + *
+ * 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. + *
+ * 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 a0059e5a07..614821513a 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 @@ -119,6 +119,120 @@ public class JDBCStorage implements org.opends.server.backends.pluggable.spi.Sto 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. + *
+ * 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. + *
+ * 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. + *
+ * 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. + *
+ * 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. + *
+ * 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); @@ -136,6 +250,14 @@ public ConfigChangeResult applyConfigurationChange(JDBCBackendCfg cfg) { 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) { @@ -343,7 +465,7 @@ private static int armedMillis(Backstop state) { 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 } } @@ -430,6 +552,7 @@ long nanoTime() { 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 @@ -454,10 +577,19 @@ private static final class Backstop { 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. + *
+ * 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
@@ -538,6 +670,11 @@ private static int backstopMillis(int seconds) {
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
@@ -557,29 +694,53 @@ private void applyBackstop(Connection con, Backstop state) {
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
@@ -610,13 +771,25 @@ private void applyBackstop(Connection con, Backstop state) {
}
/**
- * 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 {
@@ -625,7 +798,7 @@ private static void restorePrevious(Connection con, Backstop state) {
// 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;
}
}
@@ -769,6 +942,14 @@ public void open(AccessMode accessMode) throws Exception {
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
@@ -791,6 +972,7 @@ public void open(AccessMode accessMode) throws Exception {
// 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;
@@ -804,6 +986,7 @@ private void releasePool() {
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);
}
@@ -1377,6 +1560,17 @@ String readStoredComment(Connection con, Dialect dialect, String tableName) thro
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.
@@ -1393,7 +1587,7 @@ boolean updateTableStatistics(Connection con, Collection
+ * 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 9cc4d976ad..28883446df 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
@@ -178,6 +178,33 @@ public void testLoginBoundDoesNotOutliveTheLogin() throws Exception {
}
}
+ /**
+ * 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));
}