-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Conversation
…ions#1689" This reverts commit 99a5170. This commit broke the serial execution of background tasks. See: <androidannotations#1775 (comment)> Fixes androidannotations#1775. <androidannotations#1775>
BackgroundExecutor assumed that its executor was asynchronous, so that in execute(Task), a task could not be removed before it has been added to the TASKS list. However, this assumption was wrong with a synchronous executor: directExecute(…) would execute the task synchronously, leading to call Task#postExecute(), that removes the task from TASKS. Immediately after the execution, directExecute(…) would add the task to TASKS, which let the BackgroundExecutor in an incorrect state. Therefore, add the task to the TASKS list before the execution, so that it will also work with synchronous executors. Fixes androidannotations#1689. <androidannotations#1689>
@Background
serial execution
I reverted the fix that broke serial when fixing deadlock, then I provided a commit to fix the deadlock without breaking serial.
Then don't use AA or even Android at all. |
You mean that only testing is remaining for this PR?
Android is not that bad at this when you need a device like this one that needs to access network, take pictures, give sensors informations (GPS, accelerometer, etc) and so on. Doing something like this without Android would be reinventing the wheel. |
Sorry guys, obviously it is my job to merge this, and eventually create a hotfix. Unfortunately i have very busy days (i am working onsite for several weeks), so i cannot guarantee any time. @LouisCAD if this is very important to you, you can build this PR locally and use that in your project. But i will review this PR and merge this as soon as i find some time. |
Yes, this is very important for me, I'll follow your suggestion and build this PR locally. Thanks to you @rom1v and @WonderCsabo BTW, if you need to make additional tests to prevent such issue in the future, be sure to check my comment here which includes a simple sample reproducing the issue reliably. |
@rom1v would you mind taking look at #1775 (comment) and include as a test case in this PR? |
I have bad news for everyone there... 😞 repositories {
mavenLocal()
}
ext {
AAVersion = '4.1.0-SNAPSHOT'
supportLibsVersion = '24.0.0'
} I run the sample, and after I touched the FAB to launch the background tasks (which have the same serial if you look at the very simple source code), I saw that the behavior didn't change, the output is similar to this one. I double checked the sources, and yes, the two commits are in the sources I built, so they don't fix the serial not really serial bug... 😟 Note that I didn't updated my sample, so don't forget to replace the I think that a test like I did will be mandatory. It would need a way to stop the recursive background task I put in the MainActivity if it's copy pasted from my sample though. |
@rom1v Tell me if you want me to investigate the issue too. |
@rom1v I think it's the same bug. The original reporter of #1775 had an issue with |
@LouisCAD I simplified your sample to get only 1 activity and remove unrelated stuff, like "Timber": import android.app.Activity;
import android.os.Bundle;
import android.os.Handler;
import android.os.SystemClock;
import android.util.Log;
import org.androidannotations.annotations.Background;
import org.androidannotations.annotations.EActivity;
@EActivity(R.layout.my_activity)
public class MainActivity extends Activity {
@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
doDummyWork();
new Handler().postDelayed(new Runnable() {
@Override
public void run() {
for (int i = 0; i < 10; ++i) {
concurrencyTest();
}
}
}, 4000);
}
@Background
void doDummyWork() {
SystemClock.sleep(500);
doDummyWork();
}
@Background(serial = "concurrency")
void concurrencyTest() {
long threadId = Thread.currentThread().getId();
Log.d("ConcurrencyTest", "Hello from " + threadId);
SystemClock.sleep(2000);
Log.d("ConcurrencyTest", "Goodbye from " + threadId);
}
} Indeed, on the current AA version, the result is:
But with my fix, the result is:
Did you execute |
@rom1v Yes, I double checked. Did you try with my original version, just editing the |
@LouisCAD to make sure you are using the right version, you can rename the AA version and install that. Sometimes i used AA, because maven not resolved the right same snapshot version. Also double check you check out and build the right branch. mvn versions:set -DnewVersion=4.1.0-SOMETHING |
@rom1v @WonderCsabo I already double checked I checkout the right branch yesterday, in both GitHub desktop and in command line. I tried replacing with the code you ( @rom1v ) just posted, and had the same output as you. I retried with my original code, touched the FAB, and I didn't have the issue though! Sounds good for the fix, but bad for my mental integrity lol! I really don't understand why It's working today while it was not yesterday. I didn't rebuilt or edited anything apart from trying @rom1v snippet and reverting back to retry. My hypothesis are that either I didn't redeployed the app properly, or maybe an Instant Run issue which may not have invalidated some cached stuff. I'll check try on my bigger non sample project with much more |
So, the issue is likely to be related to Instant Run. I replaced the dependency in my big project to use the SNAPSHOT from this PR, and the issue still appeared after I:
I hit So although a test to check that serial is really serial would be welcome, I can say that this PR seems safe to merge and deployed as a hot fix as @WonderCsabo told earlier. Sorry for the wrong report, and thanks a lot to you @rom1v! |
Thanks for cross-testing @LouisCAD ! Yep, Instant Run can be tricky... Actually turned it off, because it not deployed the code changes correctly multiple times, and i debugged for half hours, after realised that the old code is used... |
is the bug fixed in PR, could somebody merge it? |
@WonderCsabo Can you let us know here when the 4.1 milestone which includes this fix is published, please? I guess @noinnion is waiting for this |
Hopefully i can do a release in the upcoming week. However this is an open-source project, so it could be built by anyone from this branch. |
Fine! I guess this will solve @noinnion problem |
Revert the commit provided to fix the issue #1689 because it broke the serial execution of background tasks. This fixes #1775.
Then provide another commit, that fixes #1689.
See commit messages for details.