Skip to content
This repository has been archived by the owner on Oct 4, 2019. It is now read-only.

Retain class observers #101

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Retain class observers #101

wants to merge 5 commits into from

Conversation

zintus
Copy link

@zintus zintus commented Nov 13, 2015

Observers are referenced as __unsafe_unretained, but sometimes observers released, leading to crash on notification. Fix this by retaining observers.

Other approach may be keeping week references to observers, but i've took simple way :)

@zintus
Copy link
Author

zintus commented Nov 17, 2015

Bump @PaulTaykalo

@PaulTaykalo
Copy link
Member

@zintus In case if we'll do this, no instance will be ever released, and everything will be in memory.
It's probably ok for small projects, but on big ones, you'll be out of memory very short :)
If you know what objects are not being released, then, probably, we'll be able to fix this one.

@zintus
Copy link
Author

zintus commented Nov 19, 2015

Your point is perfectly valid, it's not viable solution for big projects.

I've investigated further and did find that unsubscription fails for objects changing their class after initialization. Realm doing this, so disable notifying objects with RLM class prefix. Injecting realm classes is meaningless anyway, because live schema changes aren't supported.

@zintus
Copy link
Author

zintus commented Nov 24, 2015

@PaulTaykalo gentle bump :)

@PaulTaykalo
Copy link
Member

Sorry, @zintus I missed your commit.
Will check this one :) you can use your branch for a now. I 'll check if it's not crashing other parts of application.

Also, for next releases, I think it's better to allow user to select what objects he want to be injected.
Ping me in few days, please

@PaulTaykalo
Copy link
Member

It's good to have a project with libraries installed to check how it's working and check if it's failing or not
Probably, will make one

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants