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
Move writes to the DB to a background thread. #19
Move writes to the DB to a background thread. #19
Conversation
androidSnooper.writeThread = new HandlerThread("AndroidSnooper:Writer"); | ||
androidSnooper.writeThread.start(); | ||
androidSnooper.writeHandler = new Handler(androidSnooper.writeThread.getLooper()); | ||
|
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.
HI @nullptr2this , I think we should call androidSnooper.writeThread.quit()
to free the resources. Maybe when the app is being killed. I am not sure of the performance implications of HandlerThread
as it will keep running for a long time.
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.
When the application is killed the resource will be reclaimed anyway. Process death will result in thread death. There are no OS or inter-process resources being held so the thread will not retain any other resources when it is killed by the process being killed.
HandlerThread is a wrapper around a looper. It's essentially a single thread executor that also accepts Messages in addition to runnables. Like the main thread it will not execute any code if it has no messages or runnables in it's queue.
A natural extension to this is to provide the AndroidSnooper client the ability to provide their own executor during initialization, but as:
- The existing code was already posting a runnable to a handler (on the main thread).
- I was uncertain as to how receptive you would be to PRs. :-)
I elected to keep the mechanism handler based for this PR.
finally { | ||
database.endTransaction(); | ||
database.close(); | ||
} |
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, Just make sure SnooperRepoTest.java
is passing after this change.
Thanks, :)
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've run all of the tests and they pass locally.
Thanks @nullptr2this :) |
the latest version has been promoted to the maven central. |
Writes to the DB on the main thread cause animation jitter if even a few small network requests are in progress. I use this library for development builds in the application I work on (it's great by the way, thank you for making this open source), but a constant complaint from QA is that animations any kind of scrolling or dragging can become stuttered if a network request occurs at the same time.