jamesfredley commented on code in PR #16495:
URL: https://github.com/apache/grails-core/pull/16495#discussion_r4174981251
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -1834,6 +1851,12 @@ public void persistentEntityAdded(PersistentEntity
entity) {
*/
@Override
public void stop() {
+ synchronized (this.lifecycleMonitor) {
+ stopClients();
+ }
+ }
+
+ private void stopClients() {
if (!this.running) {
Review Comment:
`stop()` returns immediately when `running` is false, so a driver client
opened before `start()` is not closed.
`getMongoClient()` or `buildIndex()` on a datastore that has not been
started can already have created a driver client through
`RestartableMongoClient`, while `running` is still false. A later `stop()`,
including the one Spring takes before an on-refresh checkpoint, hits this
return and leaves that socket open. The same hole exists if the first `start()`
connects a client and then throws before it sets `running`.
Shutdown needs to close clients that have already been activated, even when
startup has not completed, and refuse further use until an explicit `start()`.
Please add a test that uses the client before `start()`, calls `stop()`, and
asserts the driver is closed.
##########
grails-data-mongodb/docs/src/docs/asciidoc/introduction/upgradeNotes.adoc:
##########
@@ -101,9 +101,15 @@ abstract class BookService {
Please note that with autowire by-type as the default, when multiple beans for
same type are found the application with throw Exception. Use the Spring
`@Qualifier annotation for
https://docs.spring.io/spring-framework/docs/5.3.10/reference/html/core.html#beans-autowired-annotation-qualifiers[Fine-tuning
Annotation Based Autowiring with Qualifiers].
+==== GORM Declares the MongoClient in a Spring Boot Application
Review Comment:
Non-blocking. This section covers Boot client ownership, but not the change
that applies to every datastore: constructors no longer build indexes,
including a constructor that is handed an application-owned client. Code that
assumed a finished constructor meant the indexes existed needs an explicit note
to call `start()`, or to do that work after lifecycle startup. The existing
warning that querying during bean creation defeats an on-refresh checkpoint
should stay.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -1851,51 +1874,102 @@ public void stop() {
datastore.stopIndexBuild();
}
for (MongoDatastore datastore : owningTheirClient) {
- datastore.mongo.close();
+ if (datastore.mongo instanceof RestartableMongoClient restartable)
{
+ restartable.stop();
+ }
+ else {
+ datastore.mongo.close();
+ }
datastore.clientStopped = true;
}
this.running = false;
}
/**
- * Builds a replacement for each {@link MongoClient} that {@link #stop()}
closed, using the
- * same factory the original was built with, so settings applied at
startup still apply. The
- * replacement is handed out by the connection's {@link ConnectionSource}
as well as by this
- * datastore, when that is the {@link MongoConnectionSource} the factory
creates.
+ * Connects the datastore, which is the first point at which it opens a
socket.
*
- * <p>A background index build that {@link #stop()} cut short, or that was
requested while stopped,
- * runs again on a fresh executor, on every connection.
+ * <p>The first start connects the client of every connection GORM owns
and builds the indexes the domain
+ * classes declare, unless {@code grails.mongodb.buildIndexes} is {@code
false}. Nothing before it connects: the
+ * clients GORM creates are {@link RestartableMongoClient}s, which connect
when first used, and building the
+ * datastore builds no index. So a datastore created while an application
context refreshes holds no socket
+ * until Spring starts it in {@link #LIFECYCLE_PHASE}, which is what lets
the process be checkpointed with CRaC as
+ * the context refreshes ({@code spring.context.checkpoint=onRefresh}).
Spring starts it before it publishes
+ * {@code ContextRefreshedEvent}, so the indexes are in place before
{@code BootStrap} runs and before the web
+ * server accepts a request. A datastore that nothing starts - one built
outside an application context - starts
+ * itself the first time a session is opened on it.
+ *
+ * <p>Starting after {@link #stop()} brings back each {@link MongoClient}
it stopped: a
+ * {@link RestartableMongoClient} builds a new driver client and stays the
one handed out, and any other client is
+ * replaced by one built by the same factory the original was built with,
so settings applied at startup still
+ * apply. A replacement is handed out by the connection's {@link
ConnectionSource} as well as by this datastore,
+ * when that is a {@link MongoConnectionSource}. The indexes are not built
again, since they outlive a checkpoint
+ * on the server, but a background index build that {@link #stop()} cut
short, or that was requested while
+ * stopped, runs again on a fresh executor, on every connection.
*/
@Override
public void start() {
- if (this.running) {
+ synchronized (this.lifecycleMonitor) {
+ if (this.running) {
+ return;
+ }
+ if (this.started) {
+ restartClients();
+ }
+ else {
+ startForTheFirstTime();
+ }
+ }
+ }
+
+ /**
+ * Starts a datastore that nothing has started, the first time it is used.
A datastore that has been stopped is
+ * not started here: whatever stopped it starts it again, and until then
its clients refuse to be used.
+ */
+ void ensureStarted() {
+ if (this.started || this.closed) {
return;
}
+ synchronized (this.lifecycleMonitor) {
+ if (!this.started && !this.closed && !this.starting) {
+ start();
+ }
+ }
+ }
+
+ private void startForTheFirstTime() {
+ this.starting = true;
+ try {
+ List<MongoDatastore> datastores = datastoresAndChildren();
Review Comment:
A connection registered while this first startup is running can miss its
indexes permanently. The children are snapshotted here, and `started` is set
only after the build finishes. A connection that arrives in that window is not
in the snapshot, and the listener at line 321 skips it because `started` is
still false. Later sessions call `ensureStarted()` on the already-started
parent, so the omitted build is never retried. Declared unique indexes can stay
missing.
Coordinate registration with startup and drain connections that arrived
during the first start before marking it complete. A regression test that
registers a named connection after the snapshot, and asserts its indexes exist
when startup returns, would lock this in.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -316,8 +316,9 @@ public void newConnectionSource(final
ConnectionSource<MongoClient, MongoConnect
datastoresByConnectionSource.put(connectionSource.getName(), childDatastore);
registerAllEntitiesWithEnhancer();
// Registered first and then checked: either close() has
not started, and will find this
- // child when it walks the map, or it has, and the build
is never started.
- if (!closed) {
+ // child when it walks the map, or it has, and the build
is never started. A connection
+ // added before the datastore has started is built by
start(), with the others.
+ if (!closed && started) {
Review Comment:
A connection added while the datastore is stopped still opens a socket.
`started` stays true after `stop()`, so this calls `buildIndexAutomatically()`
on a handle that is still `NEW`. That builds a driver client during the
checkpoint window. A later parent `stop()` then returns immediately because
`running` is already false, so the new socket is never closed.
Defer activation and index builds while the parent is stopped, and connect
the new handle only on the next `start()`. `BuildIndexesPerConnectionSpec`
currently expects the build to finish before restart. That expectation should
be the opposite: no driver and no index commands while stopped, then indexes
after restart.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
