Fix a crashing issue #11
Conversation
chuganzy
force-pushed
the
fix-crashing-issue
branch
from
f92f461 to
ee215ef
Compare
|
Pardon the tardiness on this response. Have measured the performance of this change? Wondering how locks are doing to impact it. |
|
Affects just a little bit - https://github.com/krisk/fuse-swift/blob/master/Example/Tests/Tests.swift#L153 originally it takes 0.040s and after applying it takes 0.042s. However since Swift's array is not thread safe, this change is required to solve a crashing issue. |
|
@krisk I was getting the same threading issues, and this did indeed fix the crash. |
| stride(from: 0, to: count, by: chunkSize).forEach { | ||
| let chunk = Array(aList[$0..<min($0 + chunkSize, count)]) | ||
| group.enter() | ||
| self.searchQueue.async { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think that submitting each block as .barrier would effectively serialize the queue.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Can someone merge this in? I've been getting this crash on my asynchronous calls and It would be awesome to have this in the master branch! |
|
I didn't see this PR before taking a stab at fixing the crashes myself, in #41. IMHO that solution is a little bit more idiomatic by using dispatch queues to serialize the writes to these arrays rather than locks, and perhaps that's more performant too, although I haven't tested. Note that that PR also fixes another problem with these async APIs, which is that they report the wrong indexes. |
|
@chuganzy curious what the other threading issues you mention at top were. I don't see if you ended up submitting a PR for those. |
|
Oops, sorry I completely missed messages on this PR. Since I recently have no time to work on this, it is great if someone else could look take over this if this issue still persists. |
6 participants
Threading issue. Just fixes crashing issue.
I found couple of other threading issues. Will create another PR to address them.