Show Ticket

Status: closed, reported by nickshanks on 2009-10-30 (other)

Unsafe code sample used in manual (and perhaps in TextMate too)

On the page http://manual.macromates.com/en/snippets#snippets
you have the following code example:

[foo autorelease];
foo = [aValue retain];

This is not thread safe. If the first line gets executed on a background thread, that thread suspended and the main thread run, and the main thread goes through a run loop cycle and flushes the autorelease pool, you will be left with a dangling pointer "foo" which could be an ivar or global, accessible from any other thread.

There are two ways to fix this. Either put an access lock around it and every other place foo is used (very unlikely), or the only thread-safe way:

id old = foo;
foo = [value retain];
[old release];

Because the assignment of a new value to foo is atomic (id is the size of a ptr), and the old value is not yet released, this is completely thread safe.
Note added by Allan Odgaard on 2009-10-30 17:34:05

The actual ‘objacc’ snippet in the Objective-C bundle is as you propose.

The manual though notes that “[accessors] often look like this”, it does not make any claims about thread-safety. It is about snippet syntax.

Apple has an entire half-hour session about pitfalls when writing Objective-C accessors, might be better to throw in a link to that in the manual than make the snippet examples more complicated. Unfortuantely the session is only available for paying ADC members.

Note added by nickshanks on 2009-10-31 09:21:45

I just think it is bad to provide an example that can crash. People who are not very familiar with how autorelease pools work wouldn't know this and may copy it, or more likely, have the validity of their current usage reinforced.

Note added by Allan Odgaard on 2009-10-31 10:17:29

But your suggested change doesn’t make it thread-safe as it makes a copy of the instance variable, assigns a new value to it, and then releases that copy. But a second thread can update the instance variable after the first thread has made the copy. This means that when the first thread releases the copy, it releases an object which has already been released (by the second thread).

Writing thread-safe code is no simple task and IMO should be avoided at all costs. Which is why I think it would be much more helpful linking to a good resource about the pitfalls (in the footnotes) rather than make the snippet examples more complex in the hopes that this will be helpful to people making their code multi-threaded w/o fully understanding what is involved in changing to a non-deterministic execution model.

As they say “give a man a fish”… :)

Note added by Allan Odgaard on 2009-10-31 14:44:22

Worth mentioning that with your suggested setter, the getter should retain + autorelease returned values.

The reason for this is that an unwritten rule (quoted by an Apple employee) says that the returned value should be valid until we get back to the event loop.

If the setter does a regular release on the instance variable and this one has been returned in the same cycle, then we release that returned value.

Also, it is not entirely clear to me from your first comment if you know that each thread has its own auto-release pool. So values auto-released in one thread cannot be “expunged” by the event loop in another thread.

Note added by nickshanks on 2009-11-01 09:23:33

"it makes a copy of the instance variable, assigns a new value to it, and then releases that copy. But a second thread can update the instance variable after the first thread has made the copy. This means that when the first thread releases the copy, it releases an object which has already been released"

Bleep. I never considered that. So now I am one of the people I spoke of! I'm trying to think of a way of avoiding this without using locks, but can't. Even if you checked the value was still the same before calling autorelease, or added an extra retain-release around it, neither would solve the problem.
Where was this ADC video you mentioned, was it one of the WWDC ones?

"each thread has its own auto-release pool. So values auto-released in one thread cannot be “expunged” by the event loop in another thread"

I did know that but somehow my brain didn't make the connection. Since you don't run an event loop on the non-main thread (because AppKit isn't thread safe), then am I right in thinking the only case my suggestion defends against is invalid?

"with your suggested setter, the getter should retain + autorelease returned values"

I've always thought that was recommended policy anyway.

Note added by Allan Odgaard on 2009-11-16 17:03:10

The video was a WWDC session, yes. It was several years ago and titled Introduction to Cocoa or similar. I don’t have access to the videos with my current ADC membership, so I can’t be more specific, sorry — but the session was from before both properties, garbage collection, and grand central dispatch, so probably a lot more has been added to the issue of setters/getters ;)

Avoiding the lock (if you want thread-safety) can only be done by using low-level operations such as “test and set” or “compare and swap”. This is normally not for the faint of heart (google ‘lock-free algorithms’).

As for wrapping getters in retain/autorelease — this can be a performance issue, e.g. if you do a lot of gets in loops (which then also has the memory overhead of effectively adding up the storage for all properties referenced), it would normally be a better tradeoff to do the autoreleasing in the setter — but of course it depends on the code (and what understanding the user has of life-time for properties fetched, e.g. incase the original object goes away).

And true that the case you wanted to workaround is invalid :)