[iOS][FIXED] Data race related to read/write on ReactMarker::logTaggedMarkerImpl
#45557
Conversation
facebook-github-bot
added
CLA Signed
Base commit: fcd526d |
There was a problem hiding this comment.
@hakonk thank you for this PR! I believe it introduces breaking changes. Is there a way have same synchronization mechanism without changing public interface of ReactMarker.h?
|
@dmytrorykun Thanks for the feedback! If had more intimate knowledge of the code base, perhaps I could see a way to do it without breaking changes. However, my motivation was to make it impossible to access the variable in a non thread safe manner. That is not possible without employing a shared lock on the call site wherever it is accessed. In my opinion, this is more prone to errors than wrapping the synchronization in static methods. Another solution is to make sure the variable is assigned before ever being read. This would possibly not need any synchronization. I have however not investigated how to do this. It would, as far as I know, mean that the JS thread would have to be created earlier in the bridge initializer, and that the logger variable is explicitly assigned on that thread. Am I getting that right? Are there other applications that rely the public interface? Also, I have not looked for possible data races in the Android implementation. I originally addressed an iOS issue, but later learned that Android related code also calls the same shared C++ code. |
|
@dmytrorykun is it more preferable to expose a shared lock via |
|
@dmytrorykun Please see fb37bf6 and 3f73819. There should no longer be breaking changes in |
This reverts commit 971c588.
hakonk
force-pushed
the
fix/data-race-logTaggedMarkerImpl
branch
from
fb37bf6
to
9dbca94
Compare
|
void JReactMarker::setLogPerfMarkerIfNeeded() {
static std::once_flag flag{};
std::call_once(flag, []() {
// Do we need a lock here?
ReactMarker::logTaggedMarkerImpl = JReactMarker::logPerfMarker;
ReactMarker::logTaggedMarkerBridgelessImpl =
JReactMarker::logPerfMarkerBridgeless;
});
}
Edit: @dmytrorykun Added a lock in the Android related as well. |
|
@dmytrorykun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@dmytrorykun I see the PR has been imported, but it has yet to be merged. Please let me know if more changes need to be made. |
|
@dmytrorykun merged this pull request in 7e41ea4. |
|
This pull request was successfully merged by @hakonk in 7e41ea4 When will my fix make it into a release? | How to file a pick request? |
Summary:
When using
TSanwhile running the Unit tests ofRNTester, there are a few data races picked up. One is described here, while this PR deals with a race related to concurrent read/write ofReactMarker::logTaggedMarkerImpl. Here is theTSanoutput:The proposed solution is to wrap
logTaggedMarkerImplin a class that has a static getter and setter wherein a read/write lock is employed. It is my understanding thatlogTaggedMarkerImplis read several times, but only assigned rarely, and thus it seems appropriate with a read/write lock. The getter and setter functions are also inlineable, such that one should not need to make an extra function call when obtaining thelogTaggedMarkerImplinstance.In order to reproduce my findings and verify fix:
git revert -n 65998835c2198b9d626160a6883744801fa056a9 83a2a3c9b4e5ea588a6cc3a9281ad385a388b84aRNTesterand its test scheme.TSanbreakpoint is hit (possibly other places in the codebase as well) when accessinglogTaggedMarkerImpl. Continue execution if other breakpoints are hit before this breakpoint.git revert --abortTSanbreakpoint does not hit said code again.Changelog:
[iOS][Fixed] Data race related to read/write on
ReactMarker::logTaggedMarkerImplTest Plan:
I believe there are existing tests that will cover the proposed changes.