codeconsole commented on code in PR #16495:
URL: https://github.com/apache/grails-core/pull/16495#discussion_r4175286833
##########
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:
Fixed in b91ec8f76d. `stop()` no longer returns early when the datastore
isn't running: it stops every client GORM owns whatever state the datastore is
in. Until the next `start()`, those clients refuse to be used, and opening a
session doesn't start the datastore. `MongoDatastoreLifecycleSpec` has the test
you asked for: it uses the client before `start()`, calls `stop()`, and asserts
the driver client is closed, use is refused, a session doesn't start the
datastore, and `start()` reconnects it. A second test covers a first `start()`
that connects and then fails on the index build. Both fail against the previous
`stop()`.
One correction: Spring doesn't call `stop()` before an on-refresh
checkpoint. It takes that checkpoint in `DefaultLifecycleProcessor.onRefresh()`
before it starts any lifecycle bean, and its `beforeCheckpoint` stops beans
only once the processor is running. So nothing closes a client something
activates while beans are being created, and the CRaC section lists that among
what defeats an on-refresh checkpoint. The hole is real for an explicit
`stop()` and for a first start that fails, and both are covered now.
##########
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:
Fixed in b91ec8f76d, as you suggested. The listener now decides under the
lifecycle lock. A connection added while the datastore is stopped has its
client stopped with the others, and nothing is built. The next `start()`
connects it and builds its indexes. I reversed the
`BuildIndexesPerConnectionSpec` expectation: while stopped there's no driver
client, no index build logged for it, no index, and using it is refused; after
`start()` it's connected and the index exists. A domain class registered while
the datastore is stopped had the same problem and is deferred the same way,
tested in `BuildIndexesLifecycleSpec`.
##########
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:
Fixed in b91ec8f76d, with a follow-up in 70517cf660. Registration now
coordinates with startup through the lifecycle lock. `start()` works in passes:
once it has connected and built every connection it knows of, it looks again,
and it sets `started` only when a pass finds no connection it hasn't already
handled. A connection another thread registers waits for `start()` and is
connected and built when it gets the lock, unless `start()` already found it
(70517cf660 stops it from being built a second time). One registered from an
index build hook on the starting thread is picked up by the next pass.
`BuildIndexesPerConnectionSpec` has a regression test for each, asserting the
indexes exist when `start()` returns. Both fail against the previous `start()`.
##########
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:
Added in 4987030acd. The GORM for MongoDB upgrade notes now have "Indexes
Are Built When the Datastore Starts, Not When It Is Created". It covers every
constructor, including one handed an application's own client, shows calling
`start()`, and notes that the first GORM session starts a datastore nothing has
started. The warning about querying during bean creation is still there. The
Grails 8.0 upgrade notes have a matching bullet that links to it.
--
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]
