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

NetworkOnMainThreadException Fix #734

Conversation

jsligh
Copy link
Collaborator

@jsligh jsligh commented Jan 11, 2024

Closes #678

@jsligh jsligh added the bug label Jan 11, 2024
@jsligh jsligh self-assigned this Jan 11, 2024
@jsligh jsligh marked this pull request as ready for review January 11, 2024 19:24
* This Async task is needed because the networkTask needs to be cancelled / destroyed
* on a background thread or it throws a NetworkOnMainThreadException
**/
private static class DestroyNetworkTaskAsyncTask extends AsyncTask<BaseNetworkTask, Void, Void> {
Copy link
Contributor

@YuriyVelichkoPI YuriyVelichkoPI Feb 15, 2024

Choose a reason for hiding this comment

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

I meant that DestroyNetworkTaskAsyncTask doesn't have much sense since it just helps to make a call to the thread not from the main thread, but still, it is not correct interprocess communication that may lead to unexpected behavior.

The networkTask.cancel(true) should do all the job.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we try to remove the DestroyNetworkTaskAsyncTask at all? As for me, it just adds one more multithread call but doesn't influence the general behavior. networks.cancel(true) should stop the thread gracefully without direct communication with the network layer so the error should not appear.

The error in the initial ticket appears due to the networkTask.destroy(); call wich triggers the network communication on the main thread.

shinwan2 and others added 3 commits March 11, 2024 20:03
Using 23.0.0 will fail the build because it requires minimum SDK 21 while Prebid still supports minimum SDK 19.
fix: Pin Google Play Services Ads to 22.6.0
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.

Please review the comment. If we can solve the issue with less code it would be great. If it appears that we still need the extra class - we will keep it. But we at least will try.

* This Async task is needed because the networkTask needs to be cancelled / destroyed
* on a background thread or it throws a NetworkOnMainThreadException
**/
private static class DestroyNetworkTaskAsyncTask extends AsyncTask<BaseNetworkTask, Void, Void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we try to remove the DestroyNetworkTaskAsyncTask at all? As for me, it just adds one more multithread call but doesn't influence the general behavior. networks.cancel(true) should stop the thread gracefully without direct communication with the network layer so the error should not appear.

The error in the initial ticket appears due to the networkTask.destroy(); call wich triggers the network communication on the main thread.

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

nitpick: later we can remove import android.os.Looper;

@jsligh jsligh merged commit 7236153 into master Apr 17, 2024
5 checks passed
@jsligh jsligh deleted the 678-2-networkonmainthreadexception-when-destroying-mediationbanneradunit branch April 17, 2024 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NetworkOnMainThreadException when destroying MediationBannerAdUnit
3 participants