Use primary read preference when reading newly peristed GridFS files #324
Conversation
a9250e3 to
859565b
Compare
| return $closure(); | ||
| } finally { | ||
| $prevTags = ! empty($prevReadPref['tagsets']) ? $prevReadPref['tagsets'] : []; | ||
| $object->setReadPreference($prevReadPref['type'], $prevTags); |
There was a problem hiding this comment.
I assume this is an unrelated refactoring since we can now use finally?
There was a problem hiding this comment.
That is correct. I included it to
a) have the methods consistent
b) preserve the original exception trace which got lost due to the additional throw.
| * @return mixed | ||
| */ | ||
| private function withPrimaryReadPreference(\Closure $closure) | ||
| { |
There was a problem hiding this comment.
Would it make sense to have a NOP-guard like Query::withReadPreference()?
There was a problem hiding this comment.
I left it out because I didn't think the overhead of changing the read preference to primary and setting it back would be bad. The alternative in this case would be to compare the original read preference and conditionally change it to master. I can change it if you think it will improve performance.
There was a problem hiding this comment.
Makes sense as-is. This keeps things simpler.
This bug can appear when working on a replica set and assigning GridFS documents a secondary read preference (e.g. via the
@ReadPreferencemapping in Doctrine MongoDB ODM). The library stores the file, then reads it from the database to fetch current metadata. If the document is using a secondary read preference, this can lead to the document not being found. Furthermore, due to insufficient error handling, this will not be a regular exception but a type error when assigning the result to theGridFSFileinstance.Last but not least, the file was read twice from the database with the result of the first read not being used, so this should also (slightly) improve performance of GridFS write operations.