Skip to content

Commit

Permalink
Use block scope thread_local static for NSProgress progress stack (#1912
Browse files Browse the repository at this point in the history
)

* Use block scope thread_local static for NSProgress progress stack:

Works around issue with file_scope thread_local in current clang. Leak check unit test added.

* cr feedback: collapse initialization

* switch to auto and return type deduction
  • Loading branch information
ehren authored and Raj Seshasankaran committed Feb 7, 2017
1 parent 63fcfd7 commit 74ee886
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 11 deletions.
16 changes: 5 additions & 11 deletions Frameworks/Foundation/NSProgress.mm
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,10 @@
bool childCreated;
};

// Stack of NSProgress objects that have becomeCurrent
thread_local static std::shared_ptr<std::stack<CurrentProgress>> s_currentProgressStack;

// Returns the stack of NSProgress objects that have becomeCurrent on the current thread, initializing it if necessary
static decltype(s_currentProgressStack) & _getProgressStackForCurrentThread() {
if (!s_currentProgressStack) {
s_currentProgressStack = std::make_shared<std::stack<CurrentProgress>>();
}

return s_currentProgressStack;
static auto& _getProgressStackForCurrentThread() {
thread_local static auto tlsCurrentProgressStack = std::make_shared<std::stack<CurrentProgress>>();
return tlsCurrentProgressStack;
}

@implementation NSProgress {
Expand Down Expand Up @@ -105,8 +99,8 @@ - (instancetype)initWithParent:(NSProgress*)parentProgressOrNil userInfo:(NSDict
// Assign parent
_parent = parentProgressOrNil;

// s_currentProgressStack must not be empty to have reached here
CurrentProgress& current = s_currentProgressStack->top();
// Progress stack must not be empty to have reached here
CurrentProgress& current = _getProgressStackForCurrentThread()->top();

// Keep track of the pendingUnitCount to increment in the parent
_parentPendingUnitCount = current.pendingUnitCountToAssign;
Expand Down
13 changes: 13 additions & 0 deletions tests/unittests/Foundation/NSProgressTests.mm
Original file line number Diff line number Diff line change
Expand Up @@ -583,3 +583,16 @@ - (void)addParentTo:(NSProgress*)child {
andExpectChangeCallbacks:nil];
EXPECT_EQ(1, kvoListener.hits);
}

TEST(NSProgress, CurrentProgressLeak) {
NSProgress* progress = [NSProgress progressWithTotalUnitCount:100];

std::thread t([&progress]() {
[progress becomeCurrentWithPendingUnitCount:50];
});

t.join();

// Progress unfinished upon thread exit. Ensure we're not keeping progress alive:
EXPECT_EQ(1, [progress retainCount]);
}

0 comments on commit 74ee886

Please sign in to comment.