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

Use a weak NSHashTable for managing open result sets #304

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

Use a weak NSHashTable for managing open result sets #304

wants to merge 1 commit into from

Conversation

blakewatters
Copy link
Contributor

This change replaces the use of an NSMutableSet containing NSValue references for managing open FMResultSet objects. This eliminates the potential for crashes due to over-releasing result sets.

This change replaces the use of an `NSMutableSet` containing `NSValue` references for managing open `FMResultSet` objects. This eliminates the potential for crashes due to over-releasing result sets.
@ccgus
Copy link
Owner

ccgus commented Oct 1, 2014

Do you have an example where a crash actually occurs?

@blakewatters
Copy link
Contributor Author

Yes, we were encountering crashes during development and are running in production on a fork with this change applied. The crash potential is pretty evident since the current implementation leverages NSValue wrapping a non-retained pointer. The hash table gives you a safe, auto-zeroing weak reference that eliminates the crash potential.

@ccgus
Copy link
Owner

ccgus commented Oct 3, 2014

I believe you- I just want a reproducible case first, so I can test this guy. It could be possible that you guys are misusing the api as well…

@clemensg
Copy link
Contributor

clemensg commented Dec 3, 2014

Even without a reproducible test case for the crash, using NSHashTable instead of NSMutableSet with NSValue-wrapped result sets is a good idea.

👍

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