Skip to content

Fix: Retain cycle (db->extension->connection->db). See #332.#335

Closed
skeeet wants to merge 1 commit into
yapstudios:masterfrom
BluechipSystems:feature/WeakActionManager
Closed

Fix: Retain cycle (db->extension->connection->db). See #332.#335
skeeet wants to merge 1 commit into
yapstudios:masterfrom
BluechipSystems:feature/WeakActionManager

Conversation

@skeeet
Copy link
Copy Markdown
Contributor

@skeeet skeeet commented Jun 8, 2016

Solution: Provide connection externally
Breaking existing API.
See #332 for discussion.

Solution: Provide connection externally
Breaking existing API.
robbiehanson added a commit that referenced this pull request Sep 8, 2016
@robbiehanson
Copy link
Copy Markdown
Contributor

I ended up fixing this in a slightly different manner.

There are technically 2 ways to fix the retain cycle:

  • provide the connection externally, and have the ActionManager store it using a weak reference
  • provide a way to tell the ActionManager to drop its internal strong reference

The weak reference is arguably the cleaner way to go about it. Although I can easily see people create a new connection just to pass it to the ActionManager init method. And then not retain the connection afterwards. And then submit a bug report saying the ActionManager doesn't work... :)

But I recently ran into another issue with the ActionManager: it was starting before I was ready for it to do so. That is, during app launch, there were components that weren't ready yet. And the YapActionItems were trying to use them.

So I added suspend/resume methods to ActionManager, that operate using a 'suspendCount'.

Long story short: I ended up implementing both techniques. So you can still pass a YDBConnection in the init method, but it's not required. And if YDBActionManager does manage its own connection (i.e. you passed a nil connection during init), you can manually stop the action manager (via suspend), and it will drop its internally managed connection.

@skeeet
Copy link
Copy Markdown
Contributor Author

skeeet commented Sep 8, 2016

Sounds like a best of two worlds solution for me :)

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.

2 participants