keep NSBundle calls on a single thread

[skurfer]

…in QSResourceManager

I think @pjrobertson was right in #1861 when he said these calls should be on a single thread. But the actual code in #1871 didn’t keep it on one thread. It just locked QSRez so multiple threads couldn’t make changes simultaneously. We should probably avoid locking frequently used singletons like QSRez unless absolutely necessary.

I’ve attempted to change this so it queues things up on a single thread instead of locking the object. It seems to prevent the freezes people are reporting.

There aren’t as many changes as it looks like. It’s mostly indentation changes for things that aren’t wrapped in @synchronized any more.

fixes #1957

[pjrobertson]

Looks good. You're right that @synchronized probably wasn't the best solution. Serial queues are much nicer :)

Since you're not dispatch_releaseing the resourceQueue, I'm going to say that #1965 needs merging first.
...oh wait, you've just merged that @skurfer. I'll give this another once over and a test. It's probably something we should get out in 1.2.x soonish

[skurfer]

Actually, we probably don’t need to wrap the places where the NSBundle object is a local variable only. How could another thread send it a message in that case?

[pjrobertson]

NSBundle is not thread safe so we do need to ensure that any calls we make to NSBundle are always on the same thread.

[skurfer]

Any calls to the same object though, right? As it says in the link…

Many objects deemed “thread-unsafe” are only unsafe to use from multiple threads. Many of these objects can be used from any thread as long as it is only one thread at a time. Objects that are specifically restricted to the main thread of an application are called out as such.

So a local variable that gets destroyed as soon as the method finishes should be safe on any thread.

[pjrobertson]

I'm not quite sure what you mean by local variable - path?
The variable in this case is the singleton NSBundle.

I'd imagine that +[NSBundle bundleWithPath:] looks something like this:

+(NSBundle *)bundleWithPath:(NSString *)path {
    NSDictionary static *bundlesDict = [NSMutableDictionary dictionary];
    if ([bundlesDict objectForKey:path]) {
            return [bundlesDict objectForKey:path];
     }
     NSBundle *b = [NSBundle _somePrivateMethodToGetBundle:path];
     [bundlesDict setObject:b forKey:path];
     return b;
}

and it's because of this set/get that it may not be thread safe. Looking at it now, I think you're referring to the NSBundle instance, I'm referring to the NSBundle shared instance. If you call [NSBundle bundeWithPath:] from multiple different threads at the same time, the same NSBundle object will be returned.

As always, Mike Ash has something on the subject :) (I think his referring to 'usage on the main thread only' was before GCD came about)

[skurfer]

Looking at it now, I think you're referring to the NSBundle instance, I'm referring to the NSBundle shared instance.

Yes, I meant the instance returned by bundleWithPath:.

If you call [NSBundle bundeWithPath:] from multiple different threads at the same time, the same NSBundle object will be returned.

If that’s true, then the code is probably fine as is. At least in this one class. We should probably have QSRegistry create a queue for bundle-related operations, then have the app always go to that thread when talking to an NSBundle.

[skurfer]

So, when you say “soonish”… 😉

[pjrobertson]

Hey, we do things slowly in Wales alright? :P

Looking over it again, I'm slightly worried that this could cause deadlocks, but I have no grounds for that. Just seeing dispatch_sync makes me worried...!

We'll see ;-)

[skurfer]

I'm slightly worried that this could cause deadlocks

Well, I hope not. The way I picture it in my head, previously you had everyone coming in from multiple directions at the same time. Now, everyone has to get in line and go in, then out, the same way. I could be wrong, but people have reported it fixing the existing hangs.