Improve NSFileManager thread safety

[triplef]

This is a shallow fix for possible crashes when concurrently calling various APIs which internally use +[NSFileManager defaultManager], such as -[NSData writeToFile:options:error:].

Guards access to the _lastError instance variable with a lock, so that concurrent invocations of NSFileManager methods modifying the variable will not e.g. cause over-release of objects.

This is mainly meant to fix crashes when no error occurs. Note however that the actual error handling is not made thread safe with this fix, i.e. if an error occurs while another method is concurrently using the file manager APIs there can still be crashes and incorrect control flow. I see two ways to fix this properly:

1. Replace the _lastError instance variable with local error variables that are passed through the method calls.
2. Replace the use of defaultManager with individual NSFileManager objects in the various APIs using NSFileManager.

Both of these seem somewhat involved, so I wanted to get this shallow fix in first. If preferred I’m happy to keep the patch locally for now.

[rfm]

I have been thinking about this. Both of the proposed long term solutions have problems:
We can't generally pass information via the method calls, because we want to be compatible with Apple, so the method calls are restricted to the parameters as defined by them.
In light of the fact that the current apple documentation days that methods of the shared manager are thread-safe, we probably shouldn't be creating lots of individual instances.

So, what do you think of changing the code to keep error information in a thread-local variable rather than as an instance variable? I think that would allow us to have instances (including the shared instance) be thread safe like the current apple implementation, avoiding either changing the methods to pass extra information or having lots of temporary instances created.

[triplef]

Ah yes, great idea.

Should I just replace the _lastError ivar with using GS_THREAD_KEY_*()? I guess a local getter/setter (lastError/setLastError:) would make sense to wrap the TLS access?

[triplef]

I guess using GSCurrentThreadDictionary() is more in line with what other codes does.

[rfm]

GSCurrentThreadDictionary() is much less efficient, but more reliable for cleanup because the dictionary and its contents will reliably be destroyed at thread end.
Since the context here is file operations, which involve I/O and can be expected to be relatively slow, I think the slower but safer code would probably be fine.

[triplef]

Thanks. I ended up using the TLS macros because it didn’t seem too involved to make that work. The only slightly tricky part is support for ARC, as the current RETAIN/RELEASE() etc. macros don’t work for this case, but I think I got it working too.

If you see any issues with this approach I can also switch out the getter/setter to use GSCurrentThreadDictionary().

[rfm]

Actually, after sleeping on it, I think you made the right choice. The automatic cleanup at the end of the thread does not ensure cleanup at the end of the lifetime of an NSFileManager instance, and we'd probably want to do that anyway. So the simpler and more efficient macros seem good.

[triplef]

Great. Is this good to merge then or did you still want to review?

[rfm]

It looks good to me. Thanks.