sbglasius commented on PR #16192: URL: https://github.com/apache/grails-core/pull/16192#issuecomment-5379638780
I reviewed this and found two things worth a follow-up. Rather than leave them as prose I pushed a single commit on top of this branch — cherry-pick it if you agree with the reasoning: **Branch:** [`fix/embedded-mongo-replica-set-review-fixes`](https://github.com/apache/grails-core/tree/fix/embedded-mongo-replica-set-review-fixes) · **commit:** [`bd099a8`](https://github.com/apache/grails-core/commit/bd099a892f7f1ab341b449016765d96055bfee2a) ### 1. `RunningMongod.restart()` returns silently when the server is up ```java public synchronized void restart() { if (this.running != null) { return; } ... ``` Via `EmbeddedMongoLifecycle` this guard is unreachable — the bean's `started` flag means `stop()`, which nulls `running`, always precedes `start()`. It is reachable through the public `RunningEmbeddedMongo` interface, and there it turns a restart into a silent nothing; the pre-guard code would at least have failed loudly on the port. The commit tears the old process down and starts the replacement instead, so the guard still covers the double-stop case it was added for without swallowing a real restart. ### 2. `EmbeddedReplicaSetSpec.freePort()` sets `reuseAddress` after the bind `ServerSocket(0)` binds in the constructor, so the assignment on the next line does nothing. Setting it before the bind would not help either: it says nothing about two forks being handed the same ephemeral port, and the probe socket has no connections to leave in `TIME_WAIT`. The real exposure is that a port lost between the offer and mongod binding it fails the whole specification, because only a server the asking JVM started is ever reused from `STARTED`. The commit drops the dead line and retries the start on a fresh port up to three times. ### Verification - `EmbeddedMongoLifecycleSpec` gains *a running server is replaced by a restart rather than fought with for its port*, which compares mongod's `serverStatus.pid` across the call. It fails against the guard it replaces and passes with the fix. - `:grails-data-mongodb-embedded:test` — 41 tests, 0 failures. - The specs extending `EmbeddedReplicaSetSpec` all pass: `MongoTransactionSpec` (10), `MongoTransactionDisabledSpec` (2), `UnifiedMongoTransactionSpec` (12), `GormSpringDataSessionSupportSpec` (1). - `codeStyle` clean on both touched modules. The retry path itself has no direct test — forcing a port collision would mean stubbing a private static in a base class, which did not seem worth reshaping the class for. Nothing else in the diff looked wrong to me: the replica-set initiation, the `WebUtils.clearGrailsWebRequest` guard and the async decorator catch all read correctly. -- 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]
