Skip to content
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

Fix @Background serial execution #1803

Merged
merged 2 commits into from Jul 17, 2016
Merged

Conversation

@rom1v
Copy link
Contributor

@rom1v rom1v commented Jun 15, 2016

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.

rom1v added 2 commits Jun 15, 2016
This reverts commit 99a5170.

This commit broke the serial execution of background tasks. See:
<#1775 (comment)>

Fixes #1775.
<#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 #1689.
<#1689>
@rom1v rom1v changed the title Fix `@Background` serial execution Fix @Background serial execution Jun 15, 2016
@LouisCAD
Copy link

@LouisCAD LouisCAD commented Jun 21, 2016

@rom1v
Does it means you choose serial over deadlock? I mean, how likely is issue #1689 to happen on a program using multiple task serials with the @Background annotation?

I'm a bit afraid as my app needs to be as fail proof as possible to not fail at saving lives.

@rom1v
Copy link
Contributor Author

@rom1v rom1v commented Jun 21, 2016

@LouisCAD

Does it means you choose serial over deadlock?

I reverted the fix that broke serial when fixing deadlock, then I provided a commit to fix the deadlock without breaking serial.

I'm a bit afraid as my app needs to be as fail proof as possible to not fail at saving lives.

Then don't use AA or even Android at all.

@LouisCAD
Copy link

@LouisCAD LouisCAD commented Jun 21, 2016

@rom1v

fix to fix the deadlock without breaking serial

You mean that only testing is remaining for this PR?

Then don't use AA or even Android at all.

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.

@WonderCsabo
Copy link
Member

@WonderCsabo WonderCsabo commented Jun 23, 2016

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.

@LouisCAD
Copy link

@LouisCAD LouisCAD commented Jun 23, 2016

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.

@WonderCsabo
Copy link
Member

@WonderCsabo WonderCsabo commented Jun 23, 2016

@rom1v would you mind taking look at #1775 (comment) and include as a test case in this PR?

@LouisCAD
Copy link

@LouisCAD LouisCAD commented Jun 23, 2016

I have bad news for everyone there... 😞
I checked out @rom1v fork, setup maven on my computer, built AA from these sources, updated my build.gradle in my sample project to use AA 4.1-SNAPSHOT from the local maven like this:

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 ext {...} portion with the snippet above before testing against it.

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.

@LouisCAD
Copy link

@LouisCAD LouisCAD commented Jun 23, 2016

@rom1v Tell me if you want me to investigate the issue too.

@rom1v
Copy link
Contributor Author

@rom1v rom1v commented Jun 23, 2016

Tell me if you want me to investigate the issue too.

@LouisCAD yes, please, because currently I have no time.

I think this PR fixes both #1775 and #1689, could you (or anyone) confirm please?
It seems you detected another bug related to serial (or is it the same?).

@LouisCAD
Copy link

@LouisCAD LouisCAD commented Jun 24, 2016

@rom1v I think it's the same bug. The original reporter of #1775 had an issue with serial not being really serial when called from another @Background annotated method. This means that another @Background method was running when the others @Background methods with serial were started, and in my sample, this is exactly what I do, I call an @Background method with serial while another @Background method is running. So the real issue is that any @Background running method will make @Background with serial method calls not serial.

@rom1v
Copy link
Contributor Author

@rom1v rom1v commented Jun 24, 2016

@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:

D/ConcurrencyTest(26023): Hello from 241
D/ConcurrencyTest(26023): Hello from 232
D/ConcurrencyTest(26023): Hello from 242
D/ConcurrencyTest(26023): Hello from 240
D/ConcurrencyTest(26023): Hello from 239
D/ConcurrencyTest(26023): Hello from 244
D/ConcurrencyTest(26023): Hello from 243
D/ConcurrencyTest(26023): Hello from 236
D/ConcurrencyTest(26023): Goodbye from 241
D/ConcurrencyTest(26023): Goodbye from 232
D/ConcurrencyTest(26023): Hello from 232
D/ConcurrencyTest(26023): Goodbye from 242
D/ConcurrencyTest(26023): Goodbye from 240
D/ConcurrencyTest(26023): Goodbye from 239
D/ConcurrencyTest(26023): Hello from 239
D/ConcurrencyTest(26023): Goodbye from 244
D/ConcurrencyTest(26023): Goodbye from 243
D/ConcurrencyTest(26023): Goodbye from 236
D/ConcurrencyTest(26023): Goodbye from 232
D/ConcurrencyTest(26023): Goodbye from 239

But with my fix, the result is:

D/ConcurrencyTest(27721): Hello from 246
D/ConcurrencyTest(27721): Goodbye from 246
D/ConcurrencyTest(27721): Hello from 244
D/ConcurrencyTest(27721): Goodbye from 244
D/ConcurrencyTest(27721): Hello from 248
D/ConcurrencyTest(27721): Goodbye from 248
D/ConcurrencyTest(27721): Hello from 248
D/ConcurrencyTest(27721): Goodbye from 248
D/ConcurrencyTest(27721): Hello from 241
D/ConcurrencyTest(27721): Goodbye from 241
D/ConcurrencyTest(27721): Hello from 241
D/ConcurrencyTest(27721): Goodbye from 241
D/ConcurrencyTest(27721): Hello from 249
D/ConcurrencyTest(27721): Goodbye from 249
D/ConcurrencyTest(27721): Hello from 246
D/ConcurrencyTest(27721): Goodbye from 246
D/ConcurrencyTest(27721): Hello from 246
D/ConcurrencyTest(27721): Goodbye from 246
D/ConcurrencyTest(27721): Hello from 245
D/ConcurrencyTest(27721): Goodbye from 245

Did you execute mvn install from the AndroidAnnotations directory after having checked out this PR?

@LouisCAD
Copy link

@LouisCAD LouisCAD commented Jun 24, 2016

@rom1v Yes, I double checked. Did you try with my original version, just editing the build.gradle to use your AA version?

@WonderCsabo
Copy link
Member

@WonderCsabo WonderCsabo commented Jun 24, 2016

@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
@LouisCAD
Copy link

@LouisCAD LouisCAD commented Jun 24, 2016

@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 @Background usages, and tell you if it's working properly.

@LouisCAD
Copy link

@LouisCAD LouisCAD commented Jun 24, 2016

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:

  • Performed a gradle sync
  • Pushed the run button to run the app on the device.

I hit Clean an Rerun 'app' item from the Run menu in Android Studio, it got redeployed to the device, and the issue is gone. I tried multiple times, and I can confirm that there was an issue that this PR solves, and there's in issue in Instant Run or something in Android Studio which doesn't redeploy updated libraries.

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!

@WonderCsabo
Copy link
Member

@WonderCsabo WonderCsabo commented Jun 24, 2016

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...

@noinnion
Copy link

@noinnion noinnion commented Jul 8, 2016

is the bug fixed in PR, could somebody merge it?

@WonderCsabo WonderCsabo merged commit 406b7d7 into androidannotations:develop Jul 17, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@WonderCsabo WonderCsabo added this to the 4.1 milestone Jul 17, 2016
@LouisCAD
Copy link

@LouisCAD LouisCAD commented Jul 17, 2016

@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

@WonderCsabo
Copy link
Member

@WonderCsabo WonderCsabo commented Jul 17, 2016

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.
Now it is merged, so the snapshot can be used if needed.

@LouisCAD
Copy link

@LouisCAD LouisCAD commented Jul 17, 2016

Fine! I guess this will solve @noinnion problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.