Skip to content

Conversation

@avi-c
Copy link
Contributor

@avi-c avi-c commented Apr 17, 2019

Wrap other cases where we mutate the pendingFetches array in the barrier queue.

Include fix to not release the DispatchQueue for tile image fetches.

@psahgal
Copy link

psahgal commented Apr 17, 2019

@avi-c Looks like you beat me to putting up a fix for this by a few minutes. 😁 (#80)

self.pendingFetchesDispatchQueue.sync(flags: .barrier) { [weak self] in
guard let self = self else { return }
self.pendingFetches.removeValue(forKey: groupID)
}
Copy link

@psahgal psahgal Apr 17, 2019

Choose a reason for hiding this comment

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

I'm not too familiar with how sync() and group.notify() work, but is it possible this could cause some sort of deadlock on the main queue? From the docs on DispatchQueue:

Attempting to synchronously execute a work item on the main queue results in deadlock.

(I used a slightly different approach for wrapping this call in #80.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure either. I'll take a look at your change. It might be the way to go.

Copy link

Choose a reason for hiding this comment

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

I closed the pull request already, but feel free to reopen it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I cannot speak into the possibility of deadlock here but this is working great for me

@camdeardorff
Copy link
Contributor

Is this stale or ready to be merged into master?

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.

3 participants