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

🐛 Protect the spacesPerId variable by a barrier #1350

Merged
merged 3 commits into from
Feb 9, 2022

Conversation

ferologics
Copy link
Contributor

Fixes Thread 1: EXC_BAD_ACCESS crash that would occur whenever multiple concurrent threads would attempt to mutate spacesPerId at the same time.

Signed-off-by: Frantisek Hetes f.hetes@gmail.com

ferologics and others added 2 commits January 26, 2022 20:01
Fixes Thread 1: EXC_BAD_ACCESS crash that would occur whenever multiple concurrent threads would attempt to mutate `spacesPerId` at the same time
@stefanceriu stefanceriu self-assigned this Feb 9, 2022
Copy link
Contributor

@stefanceriu stefanceriu left a comment

Choose a reason for hiding this comment

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

To me the problem seems to stem from loadData() setting self.graph from the processingQueue, which in turn, through its didSet sets self.spacesPerId.
getSpace() on the other hand would normally set it from the main queue.

Perhaps a simpler solution would be to internally protect getSpace() on the same processingQueue? 🤔

public func getSpace(withId spaceId: String) -> MXSpace? {
        var space: MXSpace?
        processingQueue.sync {
            space = self.spacesPerId[spaceId]
        }
        
        if space == nil, let newSpace = self.session.room(withRoomId: spaceId)?.toSpace() {
            space = newSpace
            processingQueue.sync {
                self.spacesPerId[spaceId] = newSpace
            }
        }
        return space
    }

MatrixSDK/Space/MXSpaceService.swift Outdated Show resolved Hide resolved
@ferologics
Copy link
Contributor Author

@stefanceriu maybe that could do the trick 🤔. I used a separate queue to be 100% certain that the read/write operations are not conflicting. Unfortunately I currently lack the scope to test this alternative solution you propose.

@stefanceriu
Copy link
Contributor

stefanceriu commented Feb 9, 2022

It's okay, now that you explained what you intended, I think yours is nicer. I'm happy to merge it after you make those changes.
We want to have this in the next release candidate happening today so please let me know if you can't get to it in time so that I can handle it.

And thank you for taking the time to fix this 👍

@ferologics
Copy link
Contributor Author

@stefanceriu pushing the patch shortly.

@ferologics
Copy link
Contributor Author

@stefanceriu done ✅

@stefanceriu
Copy link
Contributor

Looks great, thank you very much! Will merge as soon as the checks finish 👍

@stefanceriu stefanceriu merged commit 3d0cded into matrix-org:develop Feb 9, 2022
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.

None yet

2 participants