Skip to content
This repository has been archived by the owner on May 8, 2022. It is now read-only.

Fix a crashing issue #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix a crashing issue #11

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions Fuse/Classes/Fuse.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,7 @@ extension Fuse {
let pattern = self.createPattern(from: text)

var items = [SearchResult]()
let itemsLock = NSLock()

let group = DispatchGroup()
let count = aList.count
Expand All @@ -303,7 +304,9 @@ extension Fuse {
self.searchQueue.async {
Copy link

Choose a reason for hiding this comment

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

We're able to hit this as well - it's because the searchQueue is concurrent, so if the aList.count > chunkSize, it will be susceptible to crashing during search. This can be proven by changing chunkSize to be >= aList.count. We no longer can produce the crash when that's the case.

I think the need for NSLock could be avoided if we just change this line to:

self.searchQueue.async(group: group, qos: .userInteractive, flags: .barrier, execute: { ... })

(and remove the group.enter() and group.leave() as this will do it for us)

I haven't tested the performance of this vs. using an NSLock, but I think it's cleaner and should have at least similar performance.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that submitting each block as .barrier would effectively serialize the queue.

Copy link
Owner

Choose a reason for hiding this comment

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

I suppose that instead of locking, what we could do is have each block append to its very own array, then when all are done, consolidate these arrays into a single items array.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh. I can't immediately envision how that would look but that sounds cool.

I think that #43 might be a way to avoid any locking too.

for (index, item) in chunk.enumerated() {
if let result = self.search(pattern, in: item) {
itemsLock.lock()
items.append((index, result.score, result.ranges))
itemsLock.unlock()
}
}

Expand Down Expand Up @@ -447,6 +450,7 @@ extension Fuse {
let count = aList.count

var collectionResult = [FusableSearchResult]()
let resultLock = NSLock()

stride(from: 0, to: count, by: chunkSize).forEach {
let chunk = Array(aList[$0..<min($0 + chunkSize, count)])
Expand Down Expand Up @@ -486,11 +490,13 @@ extension Fuse {
continue
}

resultLock.lock()
collectionResult.append((
index: index,
score: totalScore / Double(scores.count),
results: propertyResults
))
resultLock.unlock()
}

group.leave()
Expand Down