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

[Cache] removeAll: remove all files in cache path #267

Merged
merged 2 commits into from
Jan 4, 2016
Merged

Conversation

hpique
Copy link
Member

@hpique hpique commented Jan 3, 2016

Remove all files in cache path no matter if the format was registered or not. Fixes #125.

}
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0)) {
let timeout = dispatch_time(DISPATCH_TIME_NOW, Int64(60 * NSEC_PER_SEC))
dispatch_group_wait(group, timeout);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't want to check the return value ? at least for log ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dcharbonnier Good call.

@dcharbonnier
Copy link
Contributor

this is suspicious :

override func tearDown() {
        sut.removeAll()
        super.tearDown()
    }

I think it should be

override func tearDown() {
  sut.removeAll() {
    super.tearDown()
  }
}

@hpique
Copy link
Member Author

hpique commented Jan 3, 2016

super.tearDown should always be called within tearDown. This should do the trick:

override func tearDown() {
    let semaphore = dispatch_semaphore_create(0)
    sut.removeData() {
        dispatch_semaphore_signal(semaphore)
    }
    dispatch_semaphore_wait(semaphore, DISPATCH_TIME_FOREVER)
    super.tearDown()
}

Which basically makes removeAllData synchronous.

@dcharbonnier
Copy link
Contributor

right, removeAllData or removeAll ?

@hpique
Copy link
Member Author

hpique commented Jan 3, 2016

Both. Same problem in DiskCacheTests.

This was referenced Jan 3, 2016
@hpique
Copy link
Member Author

hpique commented Jan 3, 2016

Actually, what I posted is wrong as it would block the main queue. Threading is hard. Will revisit in a bit.

@hpique hpique mentioned this pull request Jan 4, 2016
dcharbonnier added a commit that referenced this pull request Jan 4, 2016
[Cache] removeAll: remove all files in cache path
@dcharbonnier dcharbonnier merged commit 907ec21 into master Jan 4, 2016
@dcharbonnier dcharbonnier deleted the removeAll branch January 4, 2016 06:24
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.

2 participants