Fix crash when calling sendToView while the Presenter attaches the view

[passsy]

@k0shk0sh discovered a one in a million crash in production

Fatal Exception: java.lang.IllegalStateException: no ui thread executor available
       at net.grandcentrix.thirtyinch.TiPresenter.runOnUiThread(TiPresenter.java:371)
       at net.grandcentrix.thirtyinch.TiPresenter.sendToView(TiPresenter.java:518)
       ...

It was possible to call sendToView when the view is set but a uiThreadExecutor was not.

// attachView(view)
       ...
        mView = view;
        moveToState(State.VIEW_ATTACHED, false);
       ...

sendToView was called after mView = view; on a different Thread and before an observer was able to attach a executor

This fix introduces a new state "Running" where the view is attached and all observers are informed. Actions will be postponed until this state has been reached

[StefMa]

I don't really get the point here.

All tests are green even without your changes beside of sendToView_withView_beforeInRunningState_executesAction_without_executor.
Which means (more or less)

attachView(view) {
  sendToView(view -> view.something());
}

is not "allowed" and leads to a crash.
Ok - but: Why someone should do something like that? You can call directly view.something() because attachView(view) is called and it is safe to call it.
I agree that it should not crash. But it is not so important to me that we introduce // TODOs in our code base...

Whatever. Please provide at least one test which leads to a crash if you call sendToView(view) "outside" of attachView(view) directly...

sendToView was called after mView = view on a different Thread and before an observer was able to attach a executor

How can this happen?
As I said: Please provide one test which shows exactly this issue.

[StefMa]

See #107 (comment)

[passsy]

I provided a failing test which is sendToView_withView_beforeInRunningState_executesAction_without_executor. As I said it is a multithreading problem. I have no idea how to stop the thread before line moveToState(State.VIEW_ATTACHED, false); where normally the mUiThreadExecutor will be attached.

My workaround: Don't attach a UiThreadScheduler at all and execute code in onAttachView which will be called before attachView finishes.

How to reproduce this crash:
Do some kind of async work (network request), bring the app in the background. When the app opens again and attachView will be called you have to call sendToView exactly after mView = view;, right before a UiThreadScheduler is attached in the next line.

The TODO is there because it doesn't make a lot of sense to introduce a new TiPresenter.State as part of the public API right now. I'm already planning a complete rewrite of the observer API without the before/after super boolean.

[StefMa]

I provided a failing test

No. Not really. It is obviously that this test will fail. Because the TiPresenter#setUiThreadExecutor works "hand in hand" with the TiActivityDelegate. The test don't use the Delegate so it is obviously that the uiThreadExecutor is null.

I want to see a test like you describe:

Do some kind of async work (network request), bring the app in the background. When the app opens again and attachView will be called you have to call sendToView exactly after mView = view;, right before a UiThreadScheduler is attached in the next line.

Yes - maybe it is not unit testable. But we can start with implementation tests as well 👍

sendToView exactly after mView = view;, right before a UiThreadScheduler is attached in the next line

Maybe a stupid solution. But can't we just switch the mView = view with moveToState(State.VIEW_ATTACHED, false); ? 🤔

- mView = view;
- moveToState(State.VIEW_ATTACHED, false);
+ moveToState(State.VIEW_ATTACHED, false);
+ mView = view;

moveToState() only change the state and check if it's allowed. Plus it calls all observers.
So maybe another solution is to extract the observers to a method like notifyObservers() and call it before mView = view?! 🤔

- mView = view;
- moveToState(State.VIEW_ATTACHED, false);
+ moveToState(State.VIEW_ATTACHED, false);
+ notifyObservers();
+ mView = view;

Additional to all:
If it's just a "one in a million crash" plus you want to change the observer API anyway. Why should we rush that code in? 🤔

[k0shk0sh]

@StefMa

Apparently, its not one in a million scenario anymore, however I have actually redid something about my implementation as it seems to happen in that place only where we could simply end up with literally more than 500 commit files.

I'll monitor it more with next release.

Sent from my Samsung SM-G950F using FastHub Debug

[passsy]

add volatile, will be read from multiple threads

[passsy]

I came to the conclusion I should remove the "multithreading" unit test. Unit test shouldn't mimic multi threading scenarios.

Flip mView assignment with observer notifications

Changing the mView assignment with the observer notification is not a good idea for two reasons:

• The observer API gives you callbacks exactly before and after a lifecycle method. In this case right before/after onAttachView(view). The presenter shouldn't be in a different state (mView not set) compared to the onAttachView(view) callback.
• Existing implementations could make use of getView() in the first callback. Therefore this fix would be a breaking change. And it doesn't have to be one.

Alternative solution

I think this boolean flag solution is a minimal as it can be. Alternatively we could synchronise attachView, detachView and sendToView on the same lock. Not sure if it's a better solution

Why rush?

It's a bug and we can easily fix it without changing any API. I don't know when the new observer feature will be ready to be shipped. Better fix it now and replace the fix with a better solution later.

[k0shk0sh]

@passsy well, after last update it seems the issue still happening, I don't see that this will break any existing implementation for others, I hope it gets merged soon :)

Sent from my Samsung SM-G950F using FastHub Debug

[k0shk0sh]

@passsy @StefMa well, I guess I find my mistake.

I was calling
.doFinally(() -> sendToView(BaseMvp.FAView::hideProgress)));
instead of
.doOnComplete(() -> sendToView(BaseMvp.FAView::hideProgress)));

which caused the stream to be emitted even when the observer is disposed which I think 90% is the root cause :faceplam: will monitor this within the next release and get back on it.