Fixed TransactionTooLargeException in FormDownloadList activity #2799

grzesiek2010 · 2019-01-15T22:25:46Z

Explanation

TransactionTooLargeException is thrown when an Activity is in the process of stopping, that means that the Activity was trying to send its saved state Bundles to the system OS for safe keeping for restoration later (after a config change or process death) but that one or more of the Bundles it sent were too large. There is a maximum limit of about 1MB for all such transactions occurring at once and that limit can be reached even if no single Bundle exceeds that limit

The stacktrace I attached to the issue doesn't tell us which activity cause the issue. But after reviewing values we safe in our activities and investigating logs from firebase I'm sure it must be the FormDownloadList activity.

What has been done to verify that this works as intended?

I performed manual testing of the FormDownloadList activity.

Why is this the best possible solution? Were any other approaches considered?

First I considered using frankiesardo/icepick + livefront/bridge libraries. It would allow us to fix the issue just in a few lines since the second mentioned library has been created to fix that problem (TransactionTooLargeException). Unfortunately, we would need to add two new dependencies. frankiesardo/icepick is really popular and we would consider using that library to reduce boilerplate code but livefront/bridge has only 100 stars and might be not well tested.

I decided not to take a risk using two new dependencies. That's why I used ViewModel class.

https://developer.android.com/topic/libraries/architecture/viewmodel
The ViewModel class is designed to store and manage UI-related data in a lifecycle conscious way. The ViewModel class allows data to survive configuration changes such as screen rotations.

That means the class keeps our data until the activity is destroyed. If activity was destroyed we need to load available forms again - in this case so using ViewModel class seems reasonable. Activity is not destroyed once we rotate our device so in this case, nothing is going to change. The activity might be destroyed if we close the app using home button and our device has not much memory.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

It introduces a lot of refactoring so the whole functionality that the FormDownloadList activity provides needs to be carefully tested. Please pay attention to dialogs that activity might display.

Everything should work as before apart from the behavior after destroying the activity. If activity was destroyed we need to load available forms again.
Using Don't keep activities option would be helpful here.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
verified that any code or assets from external sources are properly credited in comments and/or in the about file.
verified that any new UI elements use theme colors. UI Components Style guidelines

yanokwa · 2019-01-17T05:49:47Z

Since this is a high risk change, I think we should save this until the next release. What do you think, @grzesiek2010?

grzesiek2010 · 2019-01-17T22:07:10Z

Ok, we can wait.

seadowg · 2019-02-06T20:24:52Z

This error is the worst 💣! Ran into it on a couple of apps. +1 for using ViewModel to solve it.

Just thinking ahead: as you point out @grzesiek2010 "The activity might be destroyed if we close the app using home button and our device has not much memory". As you say using Don't keep activities is a perfect way to test this and it should be easy to simulate the scenario by switching to another app with that enabled. If it is a problem to have the form clear in some scenarios (I saw that this Activity can be started via an Intent filter) then a nice pattern I've seen is to have the ViewModel backed by a store (could just be SharedPreferences) so you can reload values back in using a ViewModelProvider.Factory implementation.

grzesiek2010 · 2019-02-13T15:42:07Z

If it is a problem to have the form clear in some scenarios (I saw that this Activity can be started via an Intent filter) then a nice pattern I've seen is to have the ViewModel backed by a store (could just be SharedPreferences) so you can reload values back in using a ViewModelProvider.Factory implementation.

It's not a problem, we just load forms again what is ok and it's maybe even better:
let's imagine a case where a user leaves the app in the background for 2 hours and the activity is killed after returning to the form list it's better to refresh it because it might be different after a few hours.

It's a different thing but:

Ran into it on a couple of apps

You seem experienced in this area so maybe you tell us what your opinion is about:
https://github.com/frankiesardo/icepick
https://github.com/evernote/android-state

I was thinking about using it in Collect but haven't even created an issue so far.

seadowg

As far as I can see there aren't any problems with the migration from the Bundle to the ViewModel in terms of storing state for the Activity. However, a lot of individual states have been migrated so I think we need to make sure this runs through QA. I think in the future it would be best to have changes like this covered by tests of the original implementation so that we know the refactor hasn't broken anything. We can use Robolectric to test lifecycle based behavior like this but it might have been easier to do with Espresso and "Don't keep activities". Happy to chat through this at some point. I saw this was pretty common in #2794 though so it probably makes sense to just move forward with manual testing to get this in.

It's not a problem, we just load forms again

Ah right if that's the case then the only problem I can see is the user being offline when the Activity reloads. If currently the app would explode though this would certainly be an improvement!

On icepick and android-state: I've definitely seen them before. Generally when I've encountered this exception before it's because the arguments being passed to an Activity or Fragment are too large rather than a deliberate onSaveInstanceState() so the fix has been different. I do think rearchitecting toViewModel makes more sense in general than those frameworks as it's now a main line piece of Android's Jetpack framework. This means it's going to get first class support, be more in sync with other framework changes and also has great interplay with other pieces such as LiveData (which could be nice in this Activity as an aside). I'd also tend towards avoiding stateful Activity or Fragment objects as much as possible. If we really need to be reloading "session" state I think it'd be cleaner to move that into SharedPreferences or Room and have the ViewModel pattern abstract/decouple it from our UI and lifecycle code.

…ialog twice after destroying the activity

mmarciniak90 · 2019-03-05T15:12:53Z

Tested with success

I noticed two differences in behavior and I talked about them with @grzesiek2010

Do not keep activities turned ON -> User is on the Get Blank Form list with Aggregate forms -> User minimizes -> User opens ODK Collect -> form list is loaded again, Connecting to server dialog is visible
User opens Get Blank Form view and user tries to connect to Aggregate without credentials -> Dialog, where user can put username and password, is visible -> User change device orientation -> Connecting to server dialog is visible for a while

Tested on Android: 4.2, 4.4, 5.1, 6.0, 7.0, 8.1

Tested cases:

green information when new versions are available
green information when new attachments are available
proper behavior for buttons: Select/Clear All, Refresh,
proper forms are selected as a default
no internet connection
searching
sort options
changing device orientation, especially on dialog with download results

@opendatakit-bot unlabel "needs testing"
@opendatakit-bot label "behavior verified"

cooperka

A few minor PR comments, plus the below feedback (always with "do not keep activities" turned on):

When I cancel the loading dialog, leave the activity, and come back, it tries to load the forms again. Shouldn't it respect loadingCanceled?
When I check some forms, leave, and come back, the forms become un-checked (note: this doesn't happen on the Send Finalized activity for example, that one remembers the checked items).

cooperka · 2019-03-05T21:09:35Z

...p/src/main/java/org/odk/collect/android/activities/viewmodels/FormDownloadListViewModel.java

@@ -163,7 +164,7 @@ public void setUrl(String url) {
    }

    public String[] getFormIdsToDownload() {
-        return formIdsToDownload;
+        return Arrays.copyOf(formIdsToDownload, formIdsToDownload.length);


Can we make this an ArrayList like the others?

The code related to external apps is relatively new and a bit fishy to me. I don't want to touch it here because it would require testing again. There is still a pr on the collectTester's side https://github.com/grzesiek2010/collectTester/pulls I'll take a look at it then.

cooperka · 2019-03-06T15:00:25Z

...p/src/main/java/org/odk/collect/android/activities/viewmodels/FormDownloadListViewModel.java

+        formList.add(item);
+    }
+
+    public void addFormList(int index, HashMap<String, String> item) {


Let's rename to addForm (it's not adding a new list, it's adding a new item to the existing list)

cooperka · 2019-03-06T15:01:27Z

...p/src/main/java/org/odk/collect/android/activities/viewmodels/FormDownloadListViewModel.java

+    private boolean shouldExit;
+    private boolean loadingCanceled;
+    // Variables for the external app intent call
+    private boolean isDownloadOnlyMode;


Can we improve this comment? I know you just copied what was there before, but it's unclear what it actually means (and it should probably be JavaDoc).

cooperka · 2019-03-06T15:02:25Z

...p/src/main/java/org/odk/collect/android/activities/viewmodels/FormDownloadListViewModel.java

+    }
+
+    public HashMap<String, Boolean> getFormResult() {
+        return formResult;


Let's pluralize for clarity. Then the methods would be getFormResults (plural, whole map) and putFormResult (singular, single item).

grzesiek2010 · 2019-03-06T19:42:53Z

@cooperka
Thanks for your review! I added some code improvements according to your comments. When it comes to those two cases you mentioned:

When I cancel the loading dialog, leave the activity, and come back, it tries to load the forms again. Shouldn't it respect loadingCanceled?

When I check some forms, leave, and come back, the forms become un-checked (note: this doesn't happen on the Send Finalized activity for example, that one remembers the checked items).

it's because the ViewModel where we keep all data (whether a user canceled loading- case 1 and checked forms - case 2 etc) survive till destroying the activity (I described it in the description). So you use don't keep activities option which simulates killing the activity so then we need to load the data again. Somewhere above I explained that it's maybe even better... if you use that option the activity is killed immediately but in a real life in most cases it would take some time so maybe it's better that we load forms again since after a long time the content might be changed.

Once you accept the pr I'll merge it.

cooperka

I understand now, thanks for clarifying. I agree that losing state on destroy is probably fine here because it's a fairly ephemeral screen -- you use it in a single session to download some forms, so long-lived state isn't that important. LGTM.

grzesiek2010 force-pushed the COLLECT-2794 branch from 2b247b3 to f33ec84 Compare January 15, 2019 23:25

yanokwa added this to the v1.20 milestone Jan 17, 2019

yanokwa modified the milestones: v1.20, v1.21 Feb 12, 2019

yanokwa requested a review from seadowg February 12, 2019 18:39

seadowg approved these changes Feb 19, 2019

View reviewed changes

grzesiek2010 added the needs testing label Feb 22, 2019

yanokwa requested a review from cooperka March 5, 2019 00:04

grzesiek2010 added 18 commits March 5, 2019 12:27

Added FormDownloadListViewModel class

3ba2a69

Moved formNamesAndURLs to FormDownloadListViewModel

bb4cf66

Moved alertTitle to FormDownloadListViewModel

8fceb0f

Moved alertMsg to FormDownloadListViewModel

1b54fa7

Moved alertShowing to FormDownloadListViewModel

60af635

Moved setShouldExit to FormDownloadListViewModel

1bc879a

Moved formList to FormDownloadListViewModel

b20510b

Moved selectedForms to FormDownloadListViewModel

4bc6167

Moved isDownloadOnlyMode to FormDownloadListViewModel

60820bb

Moved formResult to FormDownloadListViewModel

801a75d

Moved password to FormDownloadListViewModel

9354cdf

Moved username to FormDownloadListViewModel

0628009

Moved url to FormDownloadListViewModel

c9a8074

Moved formIdsToDownload to FormDownloadListViewModel

9ebeca8

Moved formsFound to FormDownloadListViewModel

3999257

Removed unused import and method

df90b4a

Fixed broken code style rules

c1ec751

Removed unused field

c883364

grzesiek2010 added 4 commits March 5, 2019 12:27

Code improvements

c77c217

Refactored the way we create and display dialogs to avoid showing a d…

a03d573

…ialog twice after destroying the activity

Dialogs should be displayed again after activity recreation if needed

f5da267

Fixed dialogs

89e6044

grzesiek2010 force-pushed the COLLECT-2794 branch from f33ec84 to 89e6044 Compare March 5, 2019 12:43

getodk-bot added behavior verified and removed behavior verified needs testing labels Mar 5, 2019

grzesiek2010 added the behavior verified label Mar 5, 2019

cooperka reviewed Mar 6, 2019

View reviewed changes

Code improvements

7fcfdb7

cooperka approved these changes Mar 7, 2019

View reviewed changes

shobhitagarwal1612 approved these changes Mar 7, 2019

View reviewed changes

shobhitagarwal1612 merged commit bbbe27b into getodk:master Mar 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed TransactionTooLargeException in FormDownloadList activity #2799

Fixed TransactionTooLargeException in FormDownloadList activity #2799

grzesiek2010 commented Jan 15, 2019 •

edited

yanokwa commented Jan 17, 2019

grzesiek2010 commented Jan 17, 2019

seadowg commented Feb 6, 2019

grzesiek2010 commented Feb 13, 2019

seadowg left a comment

mmarciniak90 commented Mar 5, 2019

cooperka left a comment

cooperka Mar 5, 2019

grzesiek2010 Mar 6, 2019

cooperka Mar 6, 2019

grzesiek2010 Mar 6, 2019

cooperka Mar 6, 2019

grzesiek2010 Mar 6, 2019

cooperka Mar 6, 2019

grzesiek2010 Mar 6, 2019

grzesiek2010 commented Mar 6, 2019

cooperka left a comment

Fixed TransactionTooLargeException in FormDownloadList activity #2799

Fixed TransactionTooLargeException in FormDownloadList activity #2799

Conversation

grzesiek2010 commented Jan 15, 2019 • edited

Explanation

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Do we need any specific form for testing your changes? If so, please attach one.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

yanokwa commented Jan 17, 2019

grzesiek2010 commented Jan 17, 2019

seadowg commented Feb 6, 2019

grzesiek2010 commented Feb 13, 2019

seadowg left a comment

Choose a reason for hiding this comment

mmarciniak90 commented Mar 5, 2019

cooperka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzesiek2010 commented Mar 6, 2019

cooperka left a comment

Choose a reason for hiding this comment

grzesiek2010 commented Jan 15, 2019 •

edited