[SERVER-47124] Enforce initializer order for MONGO_REGISTER_TEST_COMMAND Created: 25/Mar/20  Updated: 29/Oct/23  Resolved: 25/Jun/20

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: 4.7.0

Type: Bug Priority: Major - P3
Reporter: Sara Golemon Assignee: Gabriel Marks
Resolution: Fixed Votes: 0
Labels: neweng
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Backwards Compatibility: Fully Compatible
Operating System: ALL
Sprint: Security 2020-06-29
Participants:

 Description   

The helper macro MONGO_REGISTER_TEST_COMMAND currently requests an initializer be run "whenever", that is, without prependents or dependents. Fortunately, as an implementation detail, our initializer graph interprets this as "after all the complex initializers are done. However, our implementation could just as validly do the opposite and be equally correct.

In reality, we need these registrants to run after the `--setParameter EnableTestCommands=1` flag has been evaluated otherwise we risk test-only commands not being available when they should be if the implementation ever changes.

Add a prerequisite to this macro to run after "EndStartupOptionHandling"
See: https://github.com/mongodb/mongo/blob/7eab469f87e860ce9e74918fdd5a23e943eb2673/src/mongo/db/commands.h#L1004



 Comments   
Comment by Sara Golemon [ 08/Dec/20 ]

Having seen SERVER-47778 since our last exchange, your interest makes sense here. And thanks for that, btw, looks great.

Go ahead and revert this if it simplifies things. As mentioned, I agree with you that it's not necessary given the role of "default" actually sitting somewhere in the middle of the execution order rather than my prior understanding.

Comment by Billy Donahue [ 07/Dec/20 ]

It's the only place that uses the ::mongo::defaultInitializerName() outside of init itself, so I was thinking of just resimplifying this thing down so I can get rid of ::mongo::defaultInitializerName() altogether.

I'm also adding a fair amount of documentation about the startup phases so this will not be so mysterious anymore.

Comment by Sara Golemon [ 07/Dec/20 ]

You're correct.  I had misread this loop as happening after "default" (which all the test commands are as well) which would have been incorrect.  You're right that this is a no-op and could be reverted to save a small amount of startup work.

I disagree that this change hurts code clarity though.  This change makes it more apparent that the TEST_COMMAND initializers are running after StartupOptionHandling, and have no impact to readability elsewhere.

Comment by Billy Donahue [ 07/Dec/20 ]

This didn't fix any actual problem. I am recommending that we revert it as it is a downgrade in code clarity for the MONGO_REGISTER_TEST_COMMAND macro.

The premise of the ticket was flawed. The MONGO_INITIALIZER macro does not mean "run whenever". There's a sequence point called "default" that will automatically be a prerequisite of any MONGO_INITIALIZER. Depending on "default" and "EndStartupOptionHandling" is redundant. Begin and End "StartupOptionHandling" are defined by util/options_parser/startup_options_init.cpp

All the rules in that file happen after "ValidateLocale" and before "default", so this change really was a noop.

Comment by Githook User [ 25/Jun/20 ]

Author:

{'name': 'Gabriel Marks', 'email': 'gabriel.marks@mongodb.com', 'username': 'marksg07'}

Message: SERVER-47124 Add EndStartupOptionHandling as prependent to registering test commands
Branch: master
https://github.com/mongodb/mongo/commit/a8a66b7b36d7b936d99808884fccc49db61d5697

Generated at Thu Feb 08 05:13:22 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.