-
Notifications
You must be signed in to change notification settings - Fork 84
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
NetworkOnMainThreadException Fix #734
Conversation
…to after destroy() was called.
…to after destroy() was called.
…prebid-mobile-android into 673-memory-leak-detected
Run getGlobalVisibleRect on the main thread instead of a background thread
Fixing Memory Leak
Release 2.2.0
...le-core/src/main/java/org/prebid/mobile/rendering/networking/modelcontrollers/Requester.java
Outdated
Show resolved
Hide resolved
* 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> { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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
877 Arbitrary Open RTB Params
There was a problem hiding this 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> { |
There was a problem hiding this comment.
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.
There was a problem hiding this 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;
…ationbanneradunit' of https://github.com/prebid/prebid-mobile-android into 678-2-networkonmainthreadexception-when-destroying-mediationbanneradunit
Closes #678