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

Added @WorkerThread animation because Requester.Destroy() Requires Background #704

Conversation

jsligh
Copy link
Collaborator

@jsligh jsligh commented Oct 31, 2023

Closes #678

@jsligh jsligh self-assigned this Oct 31, 2023
@jsligh jsligh linked an issue Oct 31, 2023 that may be closed by this pull request
Copy link
Contributor

@YuriyVelichkoPI YuriyVelichkoPI left a comment

Choose a reason for hiding this comment

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

LGTM

@jsligh jsligh merged commit 28aa022 into master Nov 3, 2023
@jsligh jsligh deleted the 678-networkonmainthreadexception-when-destroying-mediationbanneradunit branch November 3, 2023 16:23
@linean
Copy link
Contributor

linean commented Nov 3, 2023

I believe the suggested solution is flawed for several reasons:

  1. It impacts the public SDK API, necessitating all SDK users to rework their code to invoke the destroy method from a background thread. Introducing such breaking changes should allow for a gradual transition.

  2. The MediationBaseAdUnit.destroy method would deviate from the consistency observed in all other destroy methods within the SDK, such as MediationNativeAdUnit.destroy. Addressing this inconsistency would require comprehensive documentation updates, and I am concerned that it might still result in developer errors down the line. It's worth noting that the WorkerThread annotation doesn't enforce the use of a background thread; it only issues warnings in certain cases.

  3. Moreover, the MediationBaseAdUnit, BidLoader, Requester and other child classes are not properly synchronized, and permitting the destroy method to be called from a background thread could potentially give rise to issues like race conditions and memory leaks.

Let's try to prevent introducing numerous new issues while attempting to resolve a single one.

@YuriyVelichkoPI
Copy link
Contributor

Hi @linean! Thanks for pointing out the problems, and I'm sorry for the inattentive review. I've missed that it is a part of public API.

Indeed, this fix shouldn't force publishers to rewrite their code. And there is no logical reason to force destroying the ad unit in the background thread.

Let's reopen the ticket and think about changing the behavior correctly because this crash can happen with any ad unit due to specific circumstances.

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.

NetworkOnMainThreadException when destroying MediationBannerAdUnit
3 participants