search

prebid / prebid-mobile-android

Prebid Mobile SDK for Android applications
Apache License 2.0
58 stars 82 forks source link

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

Closed jsligh closed 8 months ago

jsligh commented 8 months ago

Closes #678

linean commented 8 months ago

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 commented 8 months ago

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.