Change jsinspector back to a shared library in the CMake build#42144
Closed
motiz88 wants to merge 1 commit intofacebook:mainfrom
Closed
Change jsinspector back to a shared library in the CMake build#42144motiz88 wants to merge 1 commit intofacebook:mainfrom
motiz88 wants to merge 1 commit intofacebook:mainfrom
Conversation
facebook-github-bot
added
CLA Signed
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D52541488 |
Base commit: 5aa425c |
…ook#42144) Summary: D51895785 changed several CMake libraries from shared to static, including `jsinspector`. This happens to be semantically incorrect in the case of `jsinspector`, as the library contains singletons which can be inadvertently duplicated due to static linking. As a result, different parts of the code can end up accessing different instances of a supposed singleton, leading to bugs. Here we revert the change to `jsinspector` (only) and add an explanatory comment to signpost this for future readers. ## More context & general principle While nothing is broken today, allowing static libraries to contain global state is brittle and breaks in surprising ways: * The upcoming diff D52231237 introduces a new dependency on `jsinspector` which builds cleanly, but causes debugging to stop working because of the duplicated singleton. * The only reason debugging currently works in the CMake build of Bridgeless is by a happy accident: the shared library `hermesinstancejni` depends on `reactnativejni` through a chain of three other libraries unrelated to debugging, and as a result, can access `reactnativejni`'s copy of `jsinspector` (see graph). {F1237835169} It seems that the safest rule of thumb, given the way React Native is currently structured, is that **singletons should live in their own shared libraries** so no call site can cause them to be duplicated through static linking. (It's reasonable to revisit this guidance if we manage to consolidate React Native into one monolithic shared library, eliminating the footgun at the source.) Changelog: [Internal] [Changed] - Change jsinspector back to a shared library in the CMake build. Reviewed By: cortinico, NickGerleman Differential Revision: D52541488
motiz88
force-pushed
the
export-D52541488
branch
from
e0089f4 to
8317800
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D52541488 |
|
This pull request was successfully merged by @motiz88 in 9ba3bc9. When will my fix make it into a release? | Upcoming Releases |
Contributor
|
This pull request has been merged in 9ba3bc9. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
D51895785 changed several CMake libraries from shared to static, including
jsinspector. This happens to be semantically incorrect in the case ofjsinspector, as the library contains singletons which can be inadvertently duplicated due to static linking. As a result, different parts of the code can end up accessing different instances of the supposed singleton, leading to bugs.Here we revert the change to
jsinspector(only) and add an explanatory comment to signpost this for future readers.More context & general principle
While nothing is broken today, allowing static libraries to contain global state is brittle and breaks in surprising ways:
jsinspectorwhich builds cleanly, but causes debugging to stop working because of the duplicated singleton.hermesinstancejnidepends onreactnativejnithrough a chain of three other libraries unrelated to debugging, and as a result, can accessreactnativejni's copy ofjsinspector(see graph).{F1237835169}
It seems that the safest rule of thumb, given the way React Native is currently structured, is that singletons should live in their own shared libraries so no call site can cause them to be duplicated through static linking. (It's reasonable to revisit this guidance if we manage to consolidate React Native into one monolithic shared library, eliminating the footgun at the source.)
Changelog:
[Internal] [Changed] - Change jsinspector back to a shared library in the CMake build.
Differential Revision: D52541488