Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Further attempts at squashing the catalog scanning crash #1321

Merged
merged 2 commits into from Jan 15, 2013

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jan 15, 2013

I think this has now just become a competition between me and skurfer to see who can create the most commits that are redundant ;-)

I think I'm winning atm :D


From #1319:

Nooooooooo!

Looks like this still isn't fixed, 4 crash logs this morning. :(
How is this possible!?

I think perhaps the problem was with the dispatch_async call in saveIndex. I've changed it around slightly now so it only uses dispatch_sync, and if it's being called on the scanQueue queue, then it won't bother trying to add it to the scanQueue again. (If I'm correct, dispatch_async will make a 'copy' of the code/iVars etc. when it's called and add them to memory for when the block can be run, whereas dispatch_sync will wait until the block can be run then make a copy of the code/iVars, so potentially the iVars in the two blocks could be different.

I've also added __block to the indexDate iVar. I remember @skurfer mentioned something about not having to do this if you're using setters, but I couldn't remember the reason why, and I think it's better to be safe

This is a fun exercise right? :D

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 15, 2013

Hang on hang on...

All the crash logs in the '3940' folder on qsapp.com are version 3939 (it's version 3940 that uploaded the crash reports), so we're OK :)

Still, I think this should still be merged. Calling saveIndex on the scanQueue blindly from the scanQueue (called in scanAndCache might perhaps cause a lock)

@skurfer
Copy link
Member

@skurfer skurfer commented Jan 15, 2013

Still, I think this should still be merged.

Agreed.

Should we go ahead and change line 529 to runOnQueueSync as well?

@skurfer
Copy link
Member

@skurfer skurfer commented Jan 15, 2013

Oh, and why don't you add the latest from Transifex as long as we're merging another pull. :-)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jan 15, 2013

OK, change pushed.

Turns out there were no Transifex files to pull (no new files over 80%
complete) so that's all fine :)

On 15 January 2013 13:42, Rob McBroom notifications@github.com wrote:

Oh, and why don't you add the latest from Transifex as long as we're
merging another pull. :-)


Reply to this email directly or view it on GitHubhttps://github.com//pull/1321#issuecomment-12267061.

skurfer added a commit that referenced this issue Jan 15, 2013
Further attempts at squashing the catalog scanning crash
@skurfer skurfer merged commit 77f7959 into quicksilver:release Jan 15, 2013
@skurfer
Copy link
Member

@skurfer skurfer commented Jan 15, 2013

Hopefully most people are on weekly updates and never even saw 3940 (or 3939). :-)

@pjrobertson pjrobertson deleted the cfs branch Jan 15, 2013
skurfer added a commit that referenced this issue Jan 15, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants