Remove global C++ objects #1054
Conversation
Telemetry API should be accessible before global C++ objects (with non-trivial constructors) are constructed. This may happen when executing code in +load in Obj-C runtime, which starts before C++ globals construction, or in case of global C++ constructors race, where the order of constructors is not guaranteed in some runtimes. The replacements for such problematic globals are accessor functions with local static objects created on demand.
The refactoring broke unit test code which accessed some globals directly
tkukielk
requested review from
kindbe,
maxgolov,
mkoscumb,
reyang,
bliptec,
lalitb and
sid-dahiya
as code owners
|
|
There was a problem hiding this comment.
This PR is introducing breaking changes to the SDK, as well as some design concerns such as returning of unique_ptr by references without a validation of their contents. I believe there is issue for removing globals in the next major release, but I am not sure how this will impact the current consumers of the SDK. Tagging @lalitb for more in-depth review.
| @@ -249,11 +254,6 @@ namespace MAT_NS_BEGIN | |||
| /// <param name="powState">A reference to an instance of a MAT::PowerSource enumeration.</param> | |||
| static void getDeviceState(NetworkCost &netCost, PowerSource &powState); | |||
|
|
|||
| /// <summary> | |||
There was a problem hiding this comment.
Why are these methods being moved to Protected? These are public methods on a public header and are breaking changes.
There was a problem hiding this comment.
I followed the comments on some of these methods and they looked like private and used only internally - especially the removeCustomProfiles(), which is not thread safe, used without a guarding lock and comments indicate it should not be exposed. But I can restore them to public so we don't restrict the visibility with this change
lib/pal/PAL.cpp
Outdated
| return debugLogMutex; | ||
| } | ||
|
|
||
| std::unique_ptr<std::fstream>& GetDebugLogStream() noexcept |
There was a problem hiding this comment.
Why are we returning unique_ptr by ref? Should this be a shared_ptr then?
There was a problem hiding this comment.
I tried to be minimally invasive with these changes - only moving globals to statically constructed on demand, otherwise preserving the same. If the code was directly accessing unique_ptr global, the static accessor just mirrors that. Having said that I can change it to shared_ptr if preferred
lib/pal/PAL.cpp
Outdated
|
|
||
| std::unique_ptr<std::fstream>& GetDebugLogStream() noexcept | ||
| { | ||
| static std::unique_ptr<std::fstream> debugLogStream; |
There was a problem hiding this comment.
Globals-Bad argument aside, is having an uninitialized unique_ptr really that expensive that we need to have a static unique_ptr that we pass around as ref? The global ptr won't matter until it is initialized, so, why add this method?
There was a problem hiding this comment.
I did not pick these globals arbitrarily. I am not trying to make a generic statement that globals are bad. I fixed only places where the code was crashing on iOS if I tried to make OneDS initialization calls in +load method before OneDS globals are constructed
|
Changes looked good to me initially, but I didn't think about the backward compatibility aspects. In existing setup, there is nothing stopping the applications to build their own logic around these global statics, and all these applications need to change with this PR. While I do see this change in the right direction, we need to carefully plan for any such breaking change as part of v4 design (which is tentatively planned for next Azure semester). @mkoscumb if you have any further comments here. |
Addressing feedback. Reverting some methods' public visibility to avoid potential unnecessary side effects for SDK consumers. Changing std::unique_ptr<std::fstream> debugLogStream to std::shared_ptr<std::fstream> debugLogStream per feedback request but the accessor still needs to return the smart pointer by reference because we place the new stream object in it or reset to nil when initializing and terminating respectively.
Telemetry API should be accessible before global C++ objects (with non-trivial constructors) are constructed. This may happen when executing code in +load in Obj-C runtime, which starts before C++ globals construction, or in case of global C++ constructors race, where the order of constructors is not guaranteed in some runtimes.
The replacements for such problematic globals are accessor functions with local static objects created on demand.