search

joshdholtz / Sentry-Android

[Deprecated] Use official "raven-java" library
https://github.com/getsentry/sentry-java
MIT License
180 stars 48 forks source link

Execute HTTP POST on a background thread #21

Closed felipeska closed 8 years ago

felipeska commented 9 years ago

Execute HTTP POST on a background thread. AsyncTask enables proper and easy use of the UI thread, in this case it is not necessary ;)

joshdholtz commented 9 years ago

Thanks for the PR! Currently out sick but I will play with this when I can 👌

joshdholtz commented 9 years ago

@felipeska If we change it from an AsyncTask to a Thread, we could also probably get rid of this logic as well, right?

https://github.com/felipeska/Sentry-Android/blob/master/src/com/joshdholtz/sentry/Sentry.java#L297

felipeska commented 9 years ago

Yes! need not run on the main thread to send post requests. This should be done in a separate thread to keep the application feel slow and that can cause a ANR. you can try to emulate slow connections and removing the timeout POST request, I will verify what you just show me to be completely safe. :muscle:

joshdholtz commented 9 years ago

@felipeska It was already performing the post in the background process. If the call was made from the UI thread, it would run the AsyncTask and do the post in the background. If the call was NOT made from the UI thread, it creates a HandlerThread and then runs the AsyncTask from there.

If we simply use a Thread though we should be able to get rid of the logic where we need to create a HandlerThread and make things a bit cleaner which is always :ok_hand: with me

felipeska commented 9 years ago

@joshdholtz I understand your point. The code is cleaner to not validate the origin of the event capture (UI,Service ... anything). Avoiding misuse of the AsyncTask. :ok_hand:

spidfire commented 9 years ago

Any progress on this?

felipeska commented 9 years ago

@spidfire a problem or bug related to this?

spidfire commented 9 years ago

Nope just wondering if this will be done or not

joshdholtz commented 9 years ago

Hey @spidfire and @felipeska! Sorry about losing track of this PR :pensive: Taking a look again at this right now