Repository navigation
@EBean(SINGLETON) and getInstance_(Context) on BackgroundThread #1507
Description
Activity
When an Activity uses a @bean that it initializes in a background Thread
This should not happen normally, e.g. when injecting beans with
@Beanannotation. So i guess you are callinggetInstance_()manually in a bg thread?Yes, exactly. Like I said, that's the only sure way I know to avoid getting hit by an initialization penalty when starting the activity. If I use
@Bean, the layout will not be shown until after initializationWell, we do not really support that. Of course that is working, but using the generated code should be avoided, and that code is not designed to be called by the client. AA's bean injection is just really simple, and only supports one use-case. We could add synchronization, but that would really slow down this method, and for no reason for most cases. @yDelouis WDYT?
Ok, if view-support goes away, for my use cases, a simple change of the code to use a local variable as below would suffice. A double init is possible, but no slow down and no undefined states of the instance.
public static Client_ getInstance_(Context context) { if (instance_ == null) { Client_ instance = new Client_(context.getApplicationContext()); instance.init_(); instance_ = instance; } return instance_; }
Would you mind elaborate on this code?
BTW, why do you still need this, even with no view support in EBeans?
I'm working on an example that just shows the problem and the no view support just makes the mitigator work better. Give me 10 minutes or so, and I'll provide an example.
Took a bit longer than expected, but:
package client; import android.app.Activity; import android.content.Context; import android.os.Bundle; import org.androidannotations.annotations.AfterInject; import org.androidannotations.annotations.Background; import org.androidannotations.annotations.EActivity; import org.androidannotations.annotations.EBean; import static org.androidannotations.annotations.EBean.Scope.Singleton; /* The generated code for reference: public static WebClient_ getInstance_(Context context) { l1 if (instance_ == null) { OnViewChangedNotifier previousNotifier = OnViewChangedNotifier.replaceNotifier(null); l2 instance_ = new WebClient_(context.getApplicationContext()); l3 instance_.init_(); OnViewChangedNotifier.replaceNotifier(previousNotifier); } l4 return instance_; } */ /* Ok, what this code is <b>supposed</b> to show is when: * 1. Activity is created * 2. Background thread Y is created and checkLoggedIn() is run * 3. User rotates screen (or something else) causing onCreate -> checkLoggedIn() to be called again but is run in Thread Z * * What happens in the threads are (Thread-line) in chronological order: * (Y-l1) true * (Y-l2) quickly done * (Y-l3) hangs here on the Thread.sleep(2000), but httpClient has not been set yet * (Z-l1) false * (Z-l4) return instance that still doesn't have httpClient set * (Z) tries to call httpClient.toString() and causes NPE * (Y-l3) complete * (Y-l4) return complete instance */ @EBean(scope = Singleton) public class WebClient { private Object httpClient; @AfterInject void foo() { try { // Simulating slow initialization Thread.sleep(2000); } catch (InterruptedException e) { } httpClient = new Object(); } public boolean isLoggedIn() { // NPE if it isn't initialized System.err.println(httpClient.toString()); return false; } } @EActivity(android.support.design.R.layout.abc_search_view) class ActivityA extends Activity { @Override protected void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); // This is called e.g. when screen is rotated checkLoggedIn(); } @Background void checkLoggedIn() { // I don't want to wait for the slow initialization, so do it in the background WebClient client = WebClient_.getInstance_(this); client.isLoggedIn(); } } // Avoid the NPE by not setting the instance until it's complete, // but can create two ProposedMitigation1_ objects with the above code class ProposedMitigation1_ extends WebClient { private static ProposedMitigation1_ instance_; private ProposedMitigation1_(Context context) { } public static ProposedMitigation1_ getInstance_(Context context) { if (instance_ == null) { ProposedMitigation1_ instance = new ProposedMitigation1_(context.getApplicationContext()); instance.init_(); instance_ = instance; } return instance_; } private void init_() { } } // Basically the same as above, but without local variable and instead running init_() in the constructor class ProposedMitigation2_ extends WebClient { private static ProposedMitigation2_ instance_; private ProposedMitigation2_(Context context) { init_(); } public static ProposedMitigation2_ getInstance_(Context context) { if (instance_ == null) { instance_ = new ProposedMitigation2_(context.getApplicationContext()); } return instance_; } private void init_() { } } // DoubleCheck so that only one instance is created but try to limit the slowdown. // If the name isn't clear enough, this is possibly correct, but not what I'd propose. It's a major // slowdown. class NotReallyProposedSolution3_ extends WebClient { private static NotReallyProposedSolution3_ instance_; private NotReallyProposedSolution3_(Context context) { } public static NotReallyProposedSolution3_ getInstance_(Context context) { NotReallyProposedSolution3_ localInstance = instance_; if (localInstance == null) { synchronized (NotReallyProposedSolution3_.class) { localInstance = instance_; if (localInstance == null) { localInstance = new NotReallyProposedSolution3_(context.getApplicationContext()); localInstance.init_(); instance_ = localInstance; } } } return instance_; } private void init_() { } }
After having an epiphany in the shower (not bath tub), I've completely turned around and I say close this with "won't fix" or something.
I think the reason it works like it does, is that this is the only way to have circular dependencies of beans. It might not be the best practice in the world, but I'm sure lots of apps using AA has them.
@tobias- yeah, we indeed support circular dependencies and that is why your mitigation cannot be implemented. Please check #792.
However, i really appreciate your work on describing this issue. Thanks!
BTW, i still do not understand why you really need to inject the bean manually in the first place. You could move the initialization off from the main thread in
@AfterInject. OK, i guess you have a reason, but i do not know it.If I do that, I've gained nothing and I have a bean with an undefined state, where I need to check+lock+wait for every instance variable I use in that bean. I finally solved it by doing the initialization like:
@Override protected void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); if (!isFinishing()) { checkLoggedIn(); } } @UiThread(propagation = ENQUEUE) void checkLoggedIn() { client = Client_.getInstance_(getApplicationContext()); doCheckLoggedIn(); } @Background void doCheckLoggedIn() {
Btw, is this what you meant? (Assuming the same doCheckLoggedIn() but remove the onCreate()
@UiThread(propagation = ENQUEUE) @AfterInject void checkLoggedIn() { client = Client_.getInstance_(getApplicationContext()); doCheckLoggedIn(); }
Just to be extra clear:
- the method doing the injection uses the MainThread
- setContentView() isn't done until after the injection is complete
- By using a background thread to do the initialization in the bean, I have to start every method in the bean with
if (notYetInitialized) { ... - The only safe way to do bean
initializationgetInstance()is in the MainThread (at least until the static view methods are gone)
Well, i meant to just inject the bean with the annotation, and initialize it later. But i understand you do not like that idea. I am not sure what objects you need to inject in the bean, but you may still use the annotation to inject the bean itself to the
Activity, then annotate the bean'safterInjectmethod with@UiThread(propagation = ENQUEUE). And in the end of the method, you can notify theActivityabout the bean is done, and you can call the neededdoCheckLoggedIn(). I do not say it is better than your current solution, it is just another approach.As you agreed, i am closing this issue now. Thanks for providing your workaround, it will help others facing similar problems.
When an Activity uses a
@Beanthat it initializes in a background Thread (afaik, there is no other way to avoid just showing the background until after initialization), the code for initializing the bean is not thread safe, and can cause (and for me, causes) Exceptions because the instance exists, but has not received a state yet.The following is a typical getInstance_() method:
What happens for me is that
instance_ = new Client()has been run and completed, but notinstance_.init(). This happens when e.g. the user rotates the screen causing a new BackgroundThread to be started.I managed to hack around the delay by using
@UiThread(propagation = ENQUEUE)andgetInstance_(Context), but I would rather not have to do all initialization of all beans on the MainThread (for performance reasons).