[Lock] Fixed reading locks from replica set secondary nodes #37140

kralos · 2020-06-08T05:34:55Z

Q	A
Branch?	5.1
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #37139
License	MIT
Doc PR	symfony/symfony-docs#13775

Force lock existence query to use readPreference=primary in case the given mongo connection is using any of the following readPreferences:

primaryPreferred
secondary
secondaryPreferred
nearest

Any of the above would fail if a secondary node is queried during a lock release.

kralos · 2020-06-08T05:42:06Z

In order to add a CI test for this we would need a replicaSet in CI. Not sure this is worth doing?

nicolas-grekas

In order to add a CI test for this we would need a replicaSet in CI. Not sure this is worth doing?

Unless there is a docker image that would allow doing that easily by using GitHub Actions, I agree.

kralos · 2020-06-09T11:59:49Z

I can't find anything that appears maintained. There's https://github.com/marketplace/actions/mongodb-in-github-actions#with-a-replica-set however their replica set is single instance (there is no secondary so we therefore can't test against it). I don't really want to create and maintain our own github action for this but might make sense in the future if we want to also test \Symfony\Component\HttpFoundation\Session\Storage\Handler\MongoDbSessionHandler::doRead which i suspect has the same issue. I will be testing this in more depth over the coming months

jmikola

Just sharing a few comments after I was pinged in #37139. Feel free to take or leave my suggestions as you wish.

jmikola · 2020-06-10T01:10:34Z

src/Symfony/Component/Lock/Store/MongoDbStore.php

-        );
-        $buildInfo = $cursor->toArray()[0];
-        $this->databaseVersion = $buildInfo->version;
+        $this->readPreference = new ReadPreference(ReadPreference::RP_PRIMARY);


Note: ReadPreference can be constructed with a string since driver version 1.3.0 (September 2017). I didn't find any reference to the mongodb extension or PHP library in composer.json so perhaps there's no minimum requirement and that's the reason for preferring the deprecated numeric constants (we hope to remove these in 2.0, per PHPC-1021).

symfony doesn't require ext-mongodb, the closest it comes is "require-dev": { "mongodb/mongodb": "~1.1" }

since "mongodb/mongodb": "1.1.0" requires ext-mongodb: ^1.2.0 i think we should probably support pre ext-mongodb: 1.3.0 (needs to support ints).

I've added detection for string const vs int const to allow both forwards and backwards compatibility with the shift from int to string constructed ReadPreference

I'm assuming in https://jira.mongodb.org/browse/PHPC-1021 you're planning to just remove ReadPreference::RP_PRIMARY and ReadPreference::PRIMARY will remain in ext-mongodb: 2.0?

symfony doesn't require ext-mongodb, the closest it comes is "require-dev": { "mongodb/mongodb": "~1.1" }

Where is this? I didn't find any reference to mongodb in https://github.com/symfony/symfony/blob/5.1/composer.json

I'm assuming in https://jira.mongodb.org/browse/PHPC-1021 you're planning to just remove ReadPreference::RP_PRIMARY and ReadPreference::PRIMARY will remain in ext-mongodb: 2.0?

Correct. The new constants would stay. The old, integer constants happen to be tied to internal libmongoc constants, which is why we want to remove them (and why the values seemingly make no sense, since they're bitmasks IIRC).

In any event, I think the ternary you have in the current diff is future-proof. 👍

Where is this? I didn't find any reference to mongodb in https://github.com/symfony/symfony/blob/5.1/composer.json

in the Component\Lock - src/Symfony/Component/Lock/composer.json

jmikola · 2020-06-10T01:11:57Z

src/Symfony/Component/Lock/Store/MongoDbStore.php

@@ -315,23 +317,15 @@ private function isDuplicateKeyException(WriteException $e): bool
        return 11000 === $code;
    }

-    private function getDatabaseVersion(): string
+    private function getReadPreference(): ReadPreference


This appears to be called exactly once in exists(). ReadPreference is a very light value object, so I'd propose just removing this method (and cached class property) in favor of constructing it inline.

jmikola · 2020-06-10T01:27:03Z

Side note: while looking at

symfony/src/Symfony/Component/Lock/Tests/Store/MongoDbStoreTest.php

Line 119 in 6d6d989

yield ['mongodb://localhost/test?collection=lock', []];

, I noticed that the tests pass a custom collection option in the MongoDB connection string. Likewise, this adapter parses a default database name from the authentication database. I realize this likely needs to remain in 5.x for BC at this point, but I'd argue against having this behavior for two reasons:

The extension uses the connection string to cache internal libmongoc client objects (for socket persistence), so overloading the string to hold extra options not used by the driver will defeat any chance of being able to efficiently re-use existing sockets.
The database name in the connection string is documented as the defaultAuthDb and fallback for the authSource URI option. Suggestion folks use it to denote a default database could lead to it inadvertently being used for authentication (over the default "admin" database) if credentials were also provided.

IMO, both collection and database should be solicited as explicit options to the storage class and not pulled from the connection string, uriOptions, or driverOptions.

kralos · 2020-06-10T01:53:55Z

Side note: while looking at

symfony/src/Symfony/Component/Lock/Tests/Store/MongoDbStoreTest.php

Line 119 in 6d6d989

yield ['mongodb://localhost/test?collection=lock', []];

, I noticed that the tests pass a custom collection option in the MongoDB connection string. Likewise, this adapter parses a default database name from the authentication database. I realize this likely needs to remain in 5.x for BC at this point, but I'd argue against having this behavior for two reasons:
* The extension uses the connection string to cache internal libmongoc client objects (for socket persistence), so overloading the string to hold extra options not used by the driver will defeat any chance of being able to efficiently re-use existing sockets.

* The database name in the connection string is documented as the [`defaultAuthDb`](https://www.php.net/manual/en/mongodb-driver-manager.construct.php) and fallback for the `authSource` URI option. Suggestion folks use it to denote a default database could lead to it inadvertently being used for authentication (over the default "admin" database) if credentials were also provided.
IMO, both collection and database should be solicited as explicit options to the storage class and not pulled from the connection string, uriOptions, or driverOptions.

regarding the use of `collection` in the connection URI's querystring

I absolutely agree we should not introduce non-standard querystring params. The reason this was added at the time was to allow Symfony\Component\Lock\Store\StoreFactory to construct a MongoDbStore from a uri alone. I think we should deprecate collection from the uri in MongoDbStore as well as uri construction via StoreFactory. Since StoreFactory doesn't support $options the only way to construct a MongoDbStore would be to pass it a \MongoDB\Collection.

regarding the use of `database` in the connection URI's path

I did a fair bit of testing with mongo cli client before implementing the current behavior. As far as i can tell I have mirrored what mongo cli does:

mongodb://server/db1 means defaultDb = db1
mongodb://user:pass@server/ means defaultDb = none / mongo --nodb and authDb = admin
mongodb://user:pass@server/db1 means defaultDb = db1 and authDb = db1
mongodb://user:pass@server/db1?authSource=db2 means defaultDb = db1 and authDb = db2

jmikola · 2020-06-10T03:04:49Z

regarding the use of database in the connection URI's path
I did a fair bit of testing with mongo cli client before implementing the current behavior. As far as i can tell I have mirrored what mongo cli does:

Indeed, the MongoDB shell does use this as its default database since its REPL always has a db context. You can chalk this up to legacy behavior. FWIW, the connection strings provided by MongoDB Atlas also include the database component (I believe they use "admin"?).

From the driver's perspective, the root context is always the client object and applications explicitly select a database or collection. The connection string spec for drivers only defines this as a default auth database and makes no mention of it having any other purpose. The server manual for connection strings says the same. Given that authSource exists, I'd encourage everyone to consider the database component obsolete (unless you're working with the shell).

Some libraries built atop drivers also use the database component as their default database (e.g. default DB for storing models in the Mongoose ODM for Node.js); however, I have the same objections about that since it could also clash with authentication.

Having said all that, I imagine none of this can be changed in 5.x. If you'd like to preemptively open a ticket to address this in 6.0, I would still suggest addressing both database and collection and perhaps expanding the scope of that TODO ticket to cover any other MongoDB adapters that might exist in Symfony. It's been some time since I contributed, but I believe similar code may exist in the session adapter.

kralos · 2020-06-10T04:06:18Z

@jmikola Thanks heaps for your insight to the usage of collection / database in mongodb connection URIs. In the interest of allowing this bugfix to proceed, I've opened a separate ticket to deprecate the behavior you've raised concerns with #37180.

I'll make sure all of this also filters down to Symfony\Component\HttpFoundation\Session\Storage\Handler\MongoDbSessionHandler

fabpot · 2020-06-10T14:19:18Z

Thank you @kralos.

…eadPreference (kralos) This PR was merged into the 5.1 branch. Discussion ---------- [Lock] #37139 Updated Lock MongoDbStore note on readPreference Fixed documentation around `readPreference` for `MongoDbStore` in `Lock` component. See symfony/symfony#37140 Commits ------- 98574f5 #37139 Updated Lock MongoDbStore note on readPreference

kralos requested a review from jderusse as a code owner June 8, 2020 05:34

carsonbot added Status: Needs Review Bug labels Jun 8, 2020

kralos changed the base branch from master to 5.1 June 8, 2020 05:35

kralos mentioned this pull request Jun 8, 2020

[Lock] #37139 Updated Lock MongoDbStore note on readPreference symfony/symfony-docs#13775

Merged

kralos force-pushed the 37139-mongodb-lock-read-preference branch from 3b04eac to a0e4f1f Compare June 8, 2020 05:54

kralos changed the title ~~#37139 Fixed reading locks from a replica set where connection reads …~~ #37139 Fixed reading locks from a replica reading from secondary nodes Jun 8, 2020

kralos changed the title ~~#37139 Fixed reading locks from a replica reading from secondary nodes~~ #37139 Fixed reading locks from replica set secondary nodes Jun 8, 2020

nicolas-grekas changed the title ~~#37139 Fixed reading locks from replica set secondary nodes~~ [Lock] Fixed reading locks from replica set secondary nodes Jun 9, 2020

nicolas-grekas added this to the 5.1 milestone Jun 9, 2020

nicolas-grekas approved these changes Jun 9, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jun 9, 2020

jmikola reviewed Jun 10, 2020

View reviewed changes

kralos mentioned this pull request Jun 10, 2020

[Lock] remove non-standard MongoDbStore URI parameters before passing to driver #37180

Closed

kralos mentioned this pull request Jun 10, 2020

[Lock] MongoDbStore deprecated extracting database/collection from MongoDB connection URI #37188

Closed

jmikola approved these changes Jun 10, 2020

View reviewed changes

[Lock] Fixed reading locks from replica set secondary nodes

ebf7eaf

fabpot force-pushed the 37139-mongodb-lock-read-preference branch from 46def68 to ebf7eaf Compare June 10, 2020 14:19

fabpot merged commit 292be5f into symfony:5.1 Jun 10, 2020

fabpot mentioned this pull request Jun 12, 2020

Release v5.1.1 #37248

Merged

kralos deleted the 37139-mongodb-lock-read-preference branch August 17, 2020 23:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lock] Fixed reading locks from replica set secondary nodes #37140

[Lock] Fixed reading locks from replica set secondary nodes #37140

kralos commented Jun 8, 2020 •

edited

kralos commented Jun 8, 2020

nicolas-grekas left a comment

kralos commented Jun 9, 2020 •

edited

jmikola left a comment

jmikola Jun 10, 2020

kralos Jun 10, 2020

jmikola Jun 10, 2020

kralos Jun 10, 2020

jmikola Jun 10, 2020

kralos Jun 10, 2020

jmikola commented Jun 10, 2020

kralos commented Jun 10, 2020 •

edited

jmikola commented Jun 10, 2020 •

edited

kralos commented Jun 10, 2020

fabpot commented Jun 10, 2020

[Lock] Fixed reading locks from replica set secondary nodes #37140

[Lock] Fixed reading locks from replica set secondary nodes #37140

Conversation

kralos commented Jun 8, 2020 • edited

kralos commented Jun 8, 2020

nicolas-grekas left a comment

Choose a reason for hiding this comment

kralos commented Jun 9, 2020 • edited

jmikola left a comment

Choose a reason for hiding this comment

jmikola Jun 10, 2020

Choose a reason for hiding this comment

kralos Jun 10, 2020

Choose a reason for hiding this comment

jmikola Jun 10, 2020

Choose a reason for hiding this comment

kralos Jun 10, 2020

Choose a reason for hiding this comment

jmikola Jun 10, 2020

Choose a reason for hiding this comment

kralos Jun 10, 2020

Choose a reason for hiding this comment

jmikola commented Jun 10, 2020

kralos commented Jun 10, 2020 • edited

regarding the use of collection in the connection URI's querystring

regarding the use of database in the connection URI's path

jmikola commented Jun 10, 2020 • edited

kralos commented Jun 10, 2020

fabpot commented Jun 10, 2020

kralos commented Jun 8, 2020 •

edited

kralos commented Jun 9, 2020 •

edited

kralos commented Jun 10, 2020 •

edited

regarding the use of `collection` in the connection URI's querystring

regarding the use of `database` in the connection URI's path

jmikola commented Jun 10, 2020 •

edited