Retain Issue - Subject retaining disposed observers

[AnthonyMillerSF]

I'm hitting a bug in my production code that is causing a retain cycle. Unfortunately I am unable to provide a code sample, as the use case is proprietary currently. But I can explain what's causing it.

I've made a comment here with a question about the code that has caused this retain cycle. I thought it was best to create an issue here as well to track the issue and the conversation around it.

Feel free to close this if you don't want to track this as an issue.

[srdanrasic]

@AnthonyMillerSF has this maybe been fixed by your PR?

[AnthonyMDev]

@srdanrasic No, this is a separate issue. I'd be happy to make a PR for this, but I wanted to get your opinion on the best way to solve it first. @trupin suggested making the _deletedObservers a set of weak references. Does that seem like the right solution to you? If so, I'll make a PR for it.

[trupin]

@AnthonyMillerSF, I will be running some tests today to check if making _deletedObservers a set of weak references works for us or not. Will let you know.

[trupin]

So @AnthonyMillerSF, after auditing the code, I think that not strongly retaining the deleted observers until receiving the next event can still cause dead locks on my side.

Could you eventually write a sample of code which reproduces the retain cycle that you see? Maybe we could try tackle the problem this way since dealing with retain cycles is usually less tricky than dead locks.

[AnthonyMDev]

Thanks so much for looking into this! I can try to write a test the reproduces the retain cycle for you next week on Monday! Appreciate your help!

[stevenkramer]

@srdanrasic @AnthonyMDev I've just started trying out ReactiveKit (liking it so far BTW) and immediately noticed this issue. I don't think making _deletedObservers weak ref the observers will help, because the use case here is that the Subject already owns the last strong ref, right? I.e. you would be in the same situation. I 'solved' this by making the pruning of observers a public API. So it's obs.dispose(); subjec.pruneObservers(); for now. Not great but it solves my problem.

[srdanrasic]

Thanks everyone for driving this forward! I've released v3.15.5 where subjects no longer retain disposed observers. Please confirm is this is still an issue.

[AnthonyMDev]

Thanks so much @srdanrasic! I'll do some testing today on this, but after looking at the fixing commit (https://github.com/DeclarativeHub/ReactiveKit/commit/0b027733862717ee048c4702d387ae25255eb182) I'm concerned this might reintroduce the deadlock that @trupin initially tried to fix. @trupin do you mind taking a look at this and verifying whether this re-creates a deadlock?

[srdanrasic]

I hope it won't, but if it does, there has to be another solution to it because retaining observers leads to silent problems.

[trupin]

@AnthonyMDev, @srdanrasic, thanks for looking into this. I will do some testing today to see if the dead lock was reintroduced.

[srdanrasic]

I've been doing a lot of testing this week and I haven't seen any issue. I'll close this for now, but we can reopen it if the issue appears again.

[trupin]

Same, here, our tests didn't show any issue.

[AnthonyMillerSF]

That's awesome. So glad this is resolved. You guys rock.

[srdanrasic]

Thanks @trupin and @AnthonyMDev for reporting and helping out on this one!