[Thread Safety] Global variables are immediately flagged by Thread Sanitizer #769

New Issue

2018-05-16T10:52:23-07:00

appleguy commented

2018-05-16 10:52:23 -07:00

(Migrated from github.com)

Report

Couldn't find any duplicate for "thread sanitizer" or "global".

When launching an app that uses Yoga concurrently, even when there is no concurrent access to a particular set of YGNodeRefs, the Thread Sanitizer triggers several warnings due to unprotected concurrent writing and reading from global variables.

Issues and Steps to Reproduce

Build and run an app that uses Yoga concurrently. This can be done with the Texture framework (texturegroup.org), or just by dispatch_async'ing layouts to various independent YGNodeRef trees.

Expected Behavior

Independent YGNodeRef trees should be thread-safe to operate on, as long as the app-provided implementation of the MeasureFunc and BaselineFunc are themselves thread-safe.

Actual Behavior

When running under TSAN, several operations are immediately flagged as not thread-safe. For more background on TSAN: https://developer.apple.com/videos/play/wwdc2016/412/

Link to Code

1st one: gNodeInstanceCount (very common)
2nd one: gCurrentGenerationCount (very common)
3rd one: gDepth (a bit less common than the above two)

Note that the C++ increment operator is not atomic: https://stackoverflow.com/a/39396999

Options to fix

Using C++ atomics would be one improvement. However this would be insufficient to create transactional integrity, e.g. if an atomic value is read and then a condition statement entered, reading the atomic again to do an operation with it is not transactional.

The best fix would be to avoid maintaining global variables if at all possible, and instead store these on the root YGNodeRef. If locking is added, it should be done with care to ensure deadlocks and thread contention are not an issue.

# Report Couldn't find any duplicate for "thread sanitizer" or "global". When launching an app that uses Yoga concurrently, even when there is no concurrent access to a particular set of YGNodeRefs, the Thread Sanitizer triggers several warnings due to unprotected concurrent writing and reading from global variables. # Issues and Steps to Reproduce Build and run an app that uses Yoga concurrently. This can be done with the Texture framework (texturegroup.org), or just by dispatch_async'ing layouts to various independent YGNodeRef trees. # Expected Behavior Independent YGNodeRef trees should be thread-safe to operate on, as long as the app-provided implementation of the MeasureFunc and BaselineFunc are themselves thread-safe. # Actual Behavior When running under TSAN, several operations are immediately flagged as not thread-safe. For more background on TSAN: https://developer.apple.com/videos/play/wwdc2016/412/ # Link to Code 1st one: gNodeInstanceCount (very common) 2nd one: gCurrentGenerationCount (very common) 3rd one: gDepth (a bit less common than the above two) Note that the C++ increment operator is not atomic: https://stackoverflow.com/a/39396999 # Options to fix Using C++ atomics would be one improvement. However this would be insufficient to create transactional integrity, e.g. if an atomic value is read and then a condition statement entered, reading the atomic again to do an operation with it is not transactional. The best fix would be to avoid maintaining global variables if at all possible, and instead store these on the root YGNodeRef. If locking is added, it should be done with care to ensure deadlocks and thread contention are not an issue. ![gnodeinstancecount unsafe global](https://user-images.githubusercontent.com/565251/40134380-16543206-58f7-11e8-98e7-284f56e2c7dc.png) ![gcurrentgenerationcount unsafe global](https://user-images.githubusercontent.com/565251/40134378-162a722c-58f7-11e8-9cc7-7efceee5aaa3.png) ![gdepth unsafe global](https://user-images.githubusercontent.com/565251/40134379-1641da70-58f7-11e8-9bb9-7a504a4ac0c2.png)

👍 2

emilsjolander commented

2018-05-17 02:39:14 -07:00

(Migrated from github.com)

Agreed. This is a known issue and we have plans on moving all this into the context / node object. Not something we are working on right now but I hope we can have a look at solving this within the next month. If you have time we would also really appreciate a PR.

👍 2

rockerhieu commented

2018-06-21 04:30:55 -07:00

(Migrated from github.com)

@emilsjolander would you mind take a look at #786? Thanks!

NickGerleman commented

2022-10-04 10:17:15 -07:00

(Migrated from github.com)

This should be resolved I believe. Please reopen if it reproes against latest Yoga.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#769