Skip to content
This repository was archived by the owner on Feb 2, 2023. It is now read-only.

These messages must be called on main.#2814

Merged
hannahmbanana merged 1 commit into
facebookarchive:masterfrom
garrettmoon:trampolineToMain
Dec 20, 2016
Merged

These messages must be called on main.#2814
hannahmbanana merged 1 commit into
facebookarchive:masterfrom
garrettmoon:trampolineToMain

Conversation

@garrettmoon

Copy link
Copy Markdown
Contributor

Resolves #2813

@garrettmoon

Copy link
Copy Markdown
Contributor Author

@nguyenhuy does this look right to you?

@nguyenhuy nguyenhuy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, these methods should be called on main. I'm down for this approach. But I have one question: Should we capture strong self in these new blocks?

@garrettmoon

Copy link
Copy Markdown
Contributor Author

@nguyenhuy it should be fine because nothing is retaining the block itself?

@hannahmbanana hannahmbanana added this to the 2.0.1 milestone Dec 20, 2016
@hannahmbanana hannahmbanana merged commit 4ae2948 into facebookarchive:master Dec 20, 2016
@nguyenhuy

Copy link
Copy Markdown
Contributor

@garrettmoon I was just worrying that in rare cases, we might end up keeping these nodes around for a bit longer than needed. But this is definitely an improvement.

@garrettmoon

Copy link
Copy Markdown
Contributor Author

@nguyenhuy oh good point! I wonder if they should be though?

@nguyenhuy

Copy link
Copy Markdown
Contributor

@garrettmoon Thinking more about it, I think we actually want them to be around. For example, an image node should be kept alive until it clears its contents (i.e cache entry, placeholder image, etc).

@appleguy

Copy link
Copy Markdown
Contributor

@nguyenhuy could you share with me (maybe in an email, once you're up tomorrow) what changed in this area to cause off-main thread calls to occur? This didn't occur in practice before 2.0, right?

@nguyenhuy

nguyenhuy commented Dec 20, 2016

Copy link
Copy Markdown
Contributor

@appleguy it's an external cause actually (#2813). Developers can call -[ASNetworkImageNode setURL:] in background which then triggers -setNeeedPreload. Since it's a legitimate use case to call -setURL: off main, I think this is necessary.

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

5 participants