Skip to content
This repository has been archived by the owner on Feb 26, 2023. It is now read-only.

Check whether a @Background task is running #1909

Open
Goutte opened this issue Dec 5, 2016 · 17 comments
Open

Check whether a @Background task is running #1909

Goutte opened this issue Dec 5, 2016 · 17 comments

Comments

@Goutte
Copy link

Goutte commented Dec 5, 2016

I can't find how to do this with AA. Is it even doable ?

Checking either by serial or id is fine by me, even if it does not actually mean the same thing, as far as I understand.

I found the BackgroundExecutor.hasSerialRunning(String serial) method, but it's private.
I'm trying to hack around with checkBgThread and a custom WrongThreadListener but it feels overly complicated and anyhow I want to run this check from the UI thread so I'm not sure this'll work.

I'm probably doing this wrong, I can't be the only one with this need ; I'll update the doc if there's a simple solution I've overlooked.


To clarify, I want to check the state of a specific background task in response to user input (a click on a button) and in event listeners (I'm using the EventBus). It was rather easily doable with Asyncs, but I'm trimming the fat with the awesome annotations.

@Goutte Goutte changed the title Check whether a @Background task is running Check whether a @Background task is running Dec 5, 2016
@dodgex
Copy link
Member

dodgex commented Dec 5, 2016

As far as i know there is currently no (public) build-in method for checking this. and i'm not sure if making BackgroundExecutor.hasSerialRunning(String serial) public instead of private is a good enough to handle this.

As a approach until there is a possible solution you could either use a flag field, a callback (that gets notified of start/stop of the task) passed as e.g. param or some start/stop events with EventBus.

@WonderCsabo wdyt would it be enough and safe to make hasSerialRunning public?

@Goutte
Copy link
Author

Goutte commented Dec 5, 2016

Thanks for your interest !

I thought about managing a flag myself, but here are cases where I suspect it would quickly become tedious to handle, or the flag would become stale. I'm canceling the task from other UI callbacks, and I'm quite unsure about what happens if the activity gets suspended because it loses focus while the task is running. I could probably handle that with onStart and onStop but it quickly gets out of hand and becomes hard to maintain, which goes against the very idea of using annotations in the first place.

I'm not in any hurry to release, I aim to learn and I can take my time to get things right ™.


It looks like the TASKS queue gets popped out of the next task before it even executes, unless it's the first one with that serial. So it appears hasSerialRunning would work, but only for the first task with that serial, which is guaranteed to be confusing.

I'm quite new to Java and Android, and this is my first partial reading of AA's code, so I may totally be wrong.

@Goutte
Copy link
Author

Goutte commented Dec 5, 2016

Never mind my previous comment ; the task looks like it's re-added afterwards to TASKS ?
Gwaaaaaah my brain is melting...

@Goutte
Copy link
Author

Goutte commented Dec 6, 2016

I'm looking into the unit-tests for BackgroundExecutor. I found ThreadActivityTest but I'm even more confused... Now I remember why I stay as far away from multithreading as I can.

While we're waiting for @WonderCsabo I'll go ahead and make hasSerialRunning public in my own fork and test it as best I can.


(much later)

On another (somewhat unrelated) note, I followed the instructions on how to set up the dev env for this lib, and hit one unmentioned snag : setting the path to the android-sdk. (I'm a noob, remember?)

I solved it with the env var, as it suggests

export ANDROID_HOME=<your_path_to_android-sdk>

Also, maybe it should be mentioned somewhere in that wiki page that java 1.8 is required. I remember seeing it mentioned elsewhere, but it's not where it matters. Nothing stackoverflow can't help you solve, still...

Other than that, congratulations on a smooth setup process ! It's so satisfying to see it do everything !

@Goutte
Copy link
Author

Goutte commented Dec 7, 2016

I went ahead and tried it out.

The tests are really ugly, but they pass. I'm not sure how to write them better ; I kind of copy-pasted and monkey-coded until the following was successful.

./mvnw test -DfailIfNoTests=false -Dtest=org.androidannotations.test.ThreadActivityTest

🙈 🙊 🙉


Also, looking through the tests, I'm pretty sure that assertion is useless, unless there's more black magic at play.

@dodgex
Copy link
Member

dodgex commented Dec 10, 2016

Hey @Goutte,

from reading the test it looks okay i think... but I'm not that deep into the multithreading "behind the scenes"... :D

May @WonderCsabo and possibly even @rom1v (he implemented the serial support) can check that too? This would be a nice feature. :D

@Goutte
Copy link
Author

Goutte commented Dec 10, 2016

Thanks for the interest, @dodgex !

I'm actually right now trying my fork into the field, and I can tell you it's not working as expected !
I don't know how to functionally test what I'm testing in the field with UI events, though.

I'm reviewing the BackgroundExecutor code in more depth as a result, trying to understand it, and I'm learning all kinds of things.

@rom1v
Copy link
Contributor

rom1v commented Dec 10, 2016

I want to check the state of a specific background task

The problem is that it is inhenrently racy.

Suppose that such a method is available, let's call it isRunning(…). You would probably use it this way:

if (isRunning(…)) {
    // do something assuming the given task is running
} else {
    // do something assuming the given task is not running
}

In that case, the assumptions may be incorrect: for example, immediately after isRunning(…) returned true, maybe the given task is not running anymore.

Could you explain a bit more your concrete need?

@Goutte
Copy link
Author

Goutte commented Dec 10, 2016

What I did before moving to annotations was (given I have an AsyncTask named finder) :
finder != null && finder.getStatus() != AsyncTask.Status.FINISHED.

I am then using this condition in my updateUi method (that can be triggered by various user actions and events) to update the status of my FAB that launches said async.

The FAB acts both as a launcher for the async (which is a rather long http request) and also as a cancel button while the request is still running. But there are other ways to launch the async, such as when geolocation is resolved, which can happen pretty much anytime, and I want the FAB to be visually updated accordingly.

I have the same racing problem with my current implementation as I would have with isRunning.
Looking back, I solved it with a delay (disgusted facepalm).

I need to think about this some more, and even run multiple threads of thought. Time for a walk. Thanks for the eye-opener.

@Goutte
Copy link
Author

Goutte commented Dec 11, 2016

There's something bugging me :

@Background(serial = "request")
void doSomeRequest() {
    // Request something via the RestClient
    // ...
    
    updateUi(); // ain't the bg thread guaranteed to be finished when we reach here ?
}

@UiThread
void updateUi() {
    if (BackgroundExecutor.hasSerialRunning("request")) {
        // make FAB a cancel button
    } else {
        // make FAB a request button
    }
}

If I launch other updateUi() from, say, click listeners or GPS listeners, ain't the two updateUi calls guaranteed to be synchronized (not sure if I'm using the right semantics here, I mean guaranteed to be run one after the other, not concurrently) as they both run in the same UI thread ? Whichever runs first is of little importance (to my needs), as long as the last one is guaranteed to run after the background task ends. Is it the case ? I thought it was, but... 😱


Also, I'm a idiot. The hasSerialRunning method works fine in the field, I just forgot to tag my background method with a serial. 🙈

@Goutte
Copy link
Author

Goutte commented Dec 11, 2016

I'm looking some more at the generated sources. Any method with @UiThread is wrapped in a UiThreadExecutor.runTask(). This suggests that this wrapping is done in the background thread. Therefore, in the method annotated with the @UiThread, the background may or may not be still running, there's no way to be 100% sure. It probably stops quite fast, but that's still a race. And that's no good. Nope. No no no. Am I getting this right ?

@dodgex
Copy link
Member

dodgex commented Dec 11, 2016

[...] ain't the two updateUi calls guaranteed to be synchronized [...] as they both run in the same UI thread ?

The @Background method is running in a background thread (-> NOT the UiThread ). when you call a method with @UiThread it uses the UiThreadExecutor to forward the call to the UiThread (-> AWAY from the background thread) by using a handler that is posting a message to the main loop (aka ui thread).

this means: that if you for example continue to do some work in your background method after calling updateUi() both methods would run in parallel.

I think in most cases doSomeRequest() should finish before updateUi() starts but that is still a very racy situation that could go well in your field tests but in production it might blow your app (depending on what you are doing)

@dodgex
Copy link
Member

dodgex commented Dec 11, 2016

In your case updating an ui element after a some heavy work I'd suggest using events.

@Background(serial = "request")
void doSomeRequest() {
   eventBus.fire(*work has started event*);

    // Request something via the RestClient
    // ...
    
   eventBus.fire(*work has finished event*);
}

@UiThread
@RegisterForEvents({*work has started event*, *work has finished event*})
void updateUi(WorkEvent event) {
    // use something like `event.isWorking()` to see what state the work is...
 }

@WonderCsabo
Copy link
Member

@Goutte regarding your notes #1909 (comment) on the instructions:

  • Java 1.8 is not required, Java 1.7 is required IMHO
  • Setting ANDROID_HOME is indeed needed, fortunately the android-maven-plugin itself warns about this. But i can add it to the instructions.

@Goutte
Copy link
Author

Goutte commented Dec 11, 2016

Thanks @dodgex ; I groked it.
Regarding your solution with events, I don't see at first glance why and how it would solve the race condition. I will enquire some more.

My evening-meditation line of thinking was, if the Task.postExecute is an equivalent to Async.onPostExecute, maybe it's possible to roll out an annotation like @AfterBackgrounds(id="optional_filter_id") where a condition such as isSerialRunning or isIdRunning would be guaranteed to be false. This is getting complicated, but... Is it doable, in your opinion ? Is it testable ?


@WonderCsabo, about Java 1.8, I don't know what to tell you ; what I've observed is that ./mvnw install raised an unsupported major.minor version 52.0 until I switched to java 8. I also had to configure jitpack to use java 8 in order for it be able to build my fork.

@WonderCsabo
Copy link
Member

@Goutte i see thanks. Actually we use Java 1.7 for compiling the code, but it seems some of the Maven plugins or Maven itself now requires Java 1.8. I will update the instructions.

@Goutte
Copy link
Author

Goutte commented Dec 11, 2016

Also, I had not seen the issue #979 . Referencing it here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants