-
Type: Improvement
-
Resolution: Unresolved
-
Priority: Major - P3
-
None
-
Affects Version/s: None
-
Component/s: None
-
Replication
We provide the following methods for accessing the ReplicationCoordinator:
- static ReplicationCoordinator* get(ServiceContext*service);
- static ReplicationCoordinator* get(ServiceContext&service);
- static ReplicationCoordinator* get(OperationContext*ctx);
Callers of these methods are inconsistent as to whether they check if the return value is null. The vast majority do not check.
In practice it seems fine to not check: mongod_main always registers a coordinator early on before calling initAndListen, as does cryptd_main, and so it seems the only place a coordinator might not be running currently is in some unit tests (where a mock coordinator can be registered - we already have a mock coordinator type for this).
We should consider improving the APIs we provide for accessing the coordinator. Returning a reference, rather than a pointer, might more clearly communicate to callers that they do not need to check for null. If there are any valid use cases to access the coordinator when it may be null (e.g. code that may run before the coordinator exists during startup) we could have a separate API method for that which returns an optional or a pointer, and document what its valid use cases are, pushing users toward the other method whenever possible.
At a minimum, if we don't want to change the API, it would be good to document when callers can/can't assume it returns a value, possibly introduce invariant(s) reinforcing that assumption, and clean up any unnecessary null checks.