Scanning/Threading fixes #1319

Merged
merged 7 commits into from Jan 14, 2013

Conversation

Projects
None yet
2 participants
Owner

pjrobertson commented Jan 13, 2013

Here's another attempt at fixing the problem.

I have reverted @skurfer's fix, and gone back to my original fix. This time, however, I am checking to make sure we're not already on the scanQueue before dispatching a block to it. If we are, then we just dispatch the block (as we're on the scanQueue)

The changes made to QSTask.m are to ensure that KVO iVars are changed on the main thread, which of course must be done for UI things

The new QSGCD class takes care of checking Queues/threads etc. The basis of it is something I've previously discussed with @skurfer (and is discussed here). If/when this is merged, I'll revert 98a7a03 and use the new runOnMainQueueSync thread I've created

Owner

pjrobertson commented Jan 13, 2013

P.S. the QSTask things are here just because I was playing with runOnMainQueueSync. They're not really related, but don't change much so I thought they'd be fine here

Owner

skurfer commented Jan 14, 2013

Yeah, I thought about it and you were right. The only way to be sure is to put them on the same thread.

With that in mind, I'm wondering if we should also change scanAndCache. The way it is now, aren't we potentially looking at a similar problem, where isScanning could be false when the method is called, but true by the time the block runs? So should we move the test for it inside the block?

Otherwise, it looks OK at first glance. I'll look it over again in the morning.

What's with all the deletions in the NIBs? Just Xcode being Xcode? :-)

Owner

pjrobertson commented Jan 14, 2013

With that in mind, I'm wondering if we should also change scanAndCache.
The way it is now, aren't we potentially looking at a similar problem,
where isScanning could be false when the method is called, but true by the
time the block runs? So should we move the test for it inside the block?

Not really, since if what you say happens, and the block is reached (say on
thread A), it'll have to wait anyway, since the queue is serial, and if the
block is already being run from somewhere else (say thread B).
Putting isScanning inside the block would do nothing at all, since the code
in the block can only be executed once at any one time.

What you say might happen, and the isScanning check wouldn't be useful in
this case - we'd scan the same thing twice (serially), but in the majority
of cases, isScanning will just cancel the block from ever being called so
scanning is only performed once - not wasting resources/time.

What's with all the deletions in the NIBs? Just Xcode being Xcode? :-)

Yep. Opened, clicked 'Fix inconsistencies'. Closed
On 14 January 2013 03:52, Rob McBroom notifications@github.com wrote:

Yeah, I thought about it and you were right. The only way to be sure is to
put them on the same thread.

With that in mind, I'm wondering if we should also change scanAndCache.
The way it is now, aren't we potentially looking at a similar problem,
where isScanning could be false when the method is called, but true by
the time the block runs? So should we move the test for it inside the block?

Otherwise look OK at first glance. I'll look it over again in the morning.

What's with all the deletions in the NIBs? Just Xcode being Xcode? :-)


Reply to this email directly or view it on GitHubhttps://github.com/quicksilver/Quicksilver/pull/1319#issuecomment-12206100.

Owner

skurfer commented Jan 14, 2013

Not really, since if what you say happens, and the block is reached (say on thread A), it'll have to wait anyway, since the queue is serial, and if the block is already being run from somewhere else (say thread B).

Ah, that's why you get paid the big bucks. :-)

What you say might happen, and the isScanning check wouldn't be useful in this case - we'd scan the same thing twice (serially), but in the majority of cases, isScanning will just cancel the block from ever being called so scanning is only performed once - not wasting resources/time.

Way to anticipate my next question: "Why even have it?" Probably worth keeping it in.

Owner

pjrobertson commented Jan 14, 2013

Dammit, I'm slow... should have figured that out! :-)

I answered it really in my last reply

but in the majority of cases when isScanning == YES, the block will get stopped from being called, saving some resources/time.
Without isScanning, scan processes would just queue up waiting to be run, so if you hit ⌘R really quickly (with QS open) you'd queue up exactly the same scan processes. If you do it now (in debug) you'll see the 'multiple scans attempted' NSLog

Owner

skurfer commented Jan 14, 2013

I answered it really in my last reply

Yeah, that's what I was trying to say. You already answered it.

@skurfer skurfer commented on the diff Jan 14, 2013

Quicksilver/Code-QuickStepCore/QSCatalogEntry.m
- return NO;
- if (!indexDate)
- [self setIndexDate:[[manager attributesOfItemAtPath:indexPath error:NULL] fileModificationDate]];
- NSNumber *modInterval = [info objectForKey:kItemModificationDate];
- if (modInterval) {
- NSDate *specDate = [NSDate dateWithTimeIntervalSinceReferenceDate:[modInterval doubleValue]];
- if ([specDate compare:indexDate] == NSOrderedDescending) return NO; //Catalog Specification is more recent than index
- }
- return [[self source] indexIsValidFromDate:indexDate forEntry:info];
+ __block BOOL isValid = YES;
+ runOnQueueSync(scanQueue,^{
+ NSFileManager *manager = [NSFileManager defaultManager];
+ NSString *indexPath = [[[pIndexLocation stringByStandardizingPath] stringByAppendingPathComponent:[self identifier]]stringByAppendingPathExtension:@"qsindex"];
+ if (![manager fileExistsAtPath:indexPath isDirectory:nil]) {
+ isValid = NO;
+ }
@skurfer

skurfer Jan 14, 2013

Owner

Why not just isValid = [manager fileExistsAtPath:indexPath isDirectory:nil]?

@pjrobertson

pjrobertson Jan 14, 2013

Owner

Dunno, I think it's probably a bit clearer (for reading), but no reason really. If you look at the old code you can see I just changed return NO to isValid = NO since blocks of course can't 'return'

@skurfer skurfer added a commit that referenced this pull request Jan 14, 2013

@skurfer skurfer Merge pull request #1319 from pjrobertson/crashFixes
Scanning/Threading fixes
848db88

@skurfer skurfer merged commit 848db88 into quicksilver:release Jan 14, 2013

@skurfer skurfer added a commit that referenced this pull request Jan 14, 2013

@skurfer skurfer update release notes to cover #1319 202d579

pjrobertson deleted the pjrobertson:crashFixes branch Jan 15, 2013

Owner

pjrobertson commented Jan 15, 2013

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment