protect some additional dictionaries #2339
Conversation
these are frequently touched when icons are loaded on background threads
|
Added another one. |
|
I guess it's very difficult for you to say for sure, but does this solve the problem? Will this likely cause hangs? Also I've just checked out We could also look at using NSCache: https://developer.apple.com/reference/foundation/nscache |
I would see it twice a day, but sometimes go weeks in between, so yeah, hard to say. If it helps, the crashes always happen during
I suppose it increases the likelihood. I feel like we’re walking a knife’s edge now between hangs and crashes. I’ll look at updating the dictionary class and look at |
| @@ -469,13 +469,13 @@ - (void)setObject:(id)object forMeta:(id)aKey { | |||
| } | |||
| } | |||
|
|
|||
| - (NSMutableDictionary *)cache { | |||
| - (QSThreadSafeMutableDictionary *)cache { | |||
| if (!cache) [self setCache:[NSMutableDictionary dictionaryWithCapacity:1]]; | |||
There was a problem hiding this comment.
Here, there's one left.
Can you move the initialization of data, meta and cache in -init ? That would at least make those accessors saner (it's something I did in #2344 but I don't mind rebasing that).
There was a problem hiding this comment.
The changes on this branch are so small. It might be simpler for everyone if I just remove the QSObject changes here and you can do what you think is best to make cache thread-safe on your branch. Let me know if that sounds good.
| @@ -105,7 +105,7 @@ typedef struct _QSObjectFlags { | |||
|
|
|||
| - (void)setDetails:(NSString *)newDetails; | |||
|
|
|||
| - (NSMutableDictionary *)cache; | |||
| - (QSThreadSafeMutableDictionary *)cache; | |||
There was a problem hiding this comment.
I think I'd prefer that not to appear in the interface. Can you keep it as NSMutableDictionary * here and silence the compiler in the property instead ?
|
A couple of new self-explanatory commits. I haven’t seen a crash related to icon loading in nearly a month using this, by the way. |
|
If there are any problems with this @tiennou, let me know in the next day or two. I want to get a release out that includes this. The recent changes should address your comments, but will require some rebasing in the QSObject clean-up branch. |
|
I'm just wondering what the implications of the switch to |
|
If something needs to stay forever, it should probably go in |
|
I didn't make myself clear enough : we don't have any control over when an |
|
Yeah, I get it, but the chances are pretty low and that’s another one where it will just get repopulated the first time something tries to use it again. I don’t have strong feelings either way. Do you want me to change it back to |
|
I think I'd be more comfortable keeping the dictionary, yes. I don't really want to add another "impossible to reproduce" class of bugs |
|
OK, force-pushed to remove those last two commits. It’s back to a thread safe dictionary. I also changed the interface (which the compiler isn’t complaining about) and I moved the initialization. I’ll test it out for a while. Most likely release tomorrow unless I hear screaming from across the Atlantic. |
I mentioned revisiting this, so here we are.
I’ve seen quite a few crashes (locally and in uploaded reports) when something tries to change a dictionary inside
loadIconForObject:(which runs on a background thread).These are the ones I see regularly. Sadly, I don’t know how we could protect everything that could possibly be accessed via icon loading, but this should cover some common cases.
Seems like we should protect
dataandmetaas well, but in practice, I don’t see any crashes related to those, so I’m leaving them.