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.

Sign up for GitHub

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

Jump to bottom

Fix race during config reload and missing null check (fixes #1194) #1195

Merged
merged 6 commits into from
Jul 29, 2018
Merged

Fix race during config reload and missing null check (fixes #1194) #1195

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6a8cc7a
Fix race during config reload and missing null check
Catfriend1 Jul 26, 2018
8ccb75f
Merge branch 'master' of https://github.com/syncthing/syncthing-andro…
Catfriend1 Jul 26, 2018
475cc0f
Merge branch 'master' of https://github.com/syncthing/syncthing-andro…
Catfriend1 Jul 26, 2018
271c91b
Merge branch 'master' of https://github.com/syncthing/syncthing-andro…
Catfriend1 Jul 27, 2018
4e40e83
Review - synchronize(mConfigLock) when mConfig is accessed
Catfriend1 Jul 29, 2018
86fa316
Review - add two breaks in RestApi
Catfriend1 Jul 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions app/src/main/java/com/nutomic/syncthingandroid/fragments/DeviceListFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import com.nutomic.syncthingandroid.activities.SyncthingActivity;
import com.nutomic.syncthingandroid.model.Device;
import com.nutomic.syncthingandroid.service.Constants;
import com.nutomic.syncthingandroid.service.RestApi;
import com.nutomic.syncthingandroid.service.SyncthingService;
import com.nutomic.syncthingandroid.views.DevicesAdapter;

Expand Down Expand Up @@ -78,10 +79,17 @@ public void onViewCreated(View view, Bundle savedInstanceState) {
*/
private void updateList() {
SyncthingActivity activity = (SyncthingActivity) getActivity();
if (activity == null || activity.getApi() == null || !activity.getApi().isConfigLoaded() ||
getView() == null || activity.isFinishing())
if (activity == null || getView() == null || activity.isFinishing()) {
return;

}
RestApi restApi = activity.getApi();
if (restApi == null || !restApi.isConfigLoaded()) {
return;
}
List<Device> devices = restApi.getDevices(false);
if (devices == null) {
return;
}
if (mAdapter == null) {
mAdapter = new DevicesAdapter(activity);
setListAdapter(mAdapter);
Expand All @@ -90,10 +98,9 @@ private void updateList() {
// Prevent scroll position reset due to list update from clear().
mAdapter.setNotifyOnChange(false);
mAdapter.clear();
List<Device> devices = activity.getApi().getDevices(false);
Collections.sort(devices, DEVICES_COMPARATOR);
mAdapter.addAll(devices);
mAdapter.updateConnections(activity.getApi());
mAdapter.updateConnections(restApi);
mAdapter.notifyDataSetChanged();
setListShown(true);
}
Expand Down
17 changes: 12 additions & 5 deletions app/src/main/java/com/nutomic/syncthingandroid/fragments/FolderListFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import com.nutomic.syncthingandroid.activities.SyncthingActivity;
import com.nutomic.syncthingandroid.model.Folder;
import com.nutomic.syncthingandroid.service.Constants;
import com.nutomic.syncthingandroid.service.RestApi;
import com.nutomic.syncthingandroid.service.SyncthingService;
import com.nutomic.syncthingandroid.views.FoldersAdapter;

Expand Down Expand Up @@ -73,10 +74,17 @@ public void onViewCreated(View view, Bundle savedInstanceState) {
*/
private void updateList() {
SyncthingActivity activity = (SyncthingActivity) getActivity();
if (activity == null || activity.getApi() == null || !activity.getApi().isConfigLoaded() ||
getView() == null || activity.isFinishing())
if (activity == null || getView() == null || activity.isFinishing()) {
return;

}
RestApi restApi = activity.getApi();
if (restApi == null || !restApi.isConfigLoaded()) {
return;
}
List<Folder> folders = restApi.getFolders();
if (folders == null) {
return;
}
if (mAdapter == null) {
mAdapter = new FoldersAdapter(activity);
setListAdapter(mAdapter);
Expand All @@ -85,9 +93,8 @@ private void updateList() {
// Prevent scroll position reset due to list update from clear().
mAdapter.setNotifyOnChange(false);
mAdapter.clear();
List<Folder> folders = activity.getApi().getFolders();
mAdapter.addAll(folders);
mAdapter.updateFolderStatus(activity.getApi());
mAdapter.updateFolderStatus(restApi);
mAdapter.notifyDataSetChanged();
setListShown(true);
}
Expand Down
15 changes: 13 additions & 2 deletions app/src/main/java/com/nutomic/syncthingandroid/service/RestApi.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,11 @@ public interface OnResultListener2<T, R> {
*/
private final Object mAsyncQueryCompleteLock = new Object();

/**
* Object that must be locked upon accessing mConfig
*/
private final Object mConfigLock = new Object();

/**
* Stores the latest result of {@link #getFolderStatus} for each folder
*/
Expand Down Expand Up @@ -199,7 +204,9 @@ public void reloadConfig() {
}

private void onReloadConfigComplete(String result) {
mConfig = new Gson().fromJson(result, Config.class);
synchronized(mConfigLock) {
mConfig = new Gson().fromJson(result, Config.class);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should just use private synchronized void on all functions that touch the config, as you added a lock in one place but not others.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(all functions) I disagree. That can cause deadlocks as many modules call out to restApi.getFolders() restApi.getDevices() from other threads and the config may get reloaded on event. Improving this too fast may introduce more issues than we currently have and I cannot prove easily by testing that it improves the situation. I'll propose fix PR's at that points where crashes already occured (see GPlay console) and I can do some testing on a limited code part or module (FolderFragment, DeviceFragment in this case). When I discover more problems, I'll add more syncronization. To reach the goal that all stuff in syncthing-android is synchronized properly, the original author or a "rewriter" would have had to think about almost each edge case and put the stuff in as the app grew up. This cannot be done "overnight".

( about using synchronized void functions ) Depends on point of view, I'd stick with an explicit lock where it's needed as it can easily screw up if someone else bumps a line in and oversees the synchronized part. I'm more convenient at considering whole synchronized functions "bad practice" in java.

This PR should only address the most error-prone part that a fragment wishes to know if the config is loaded in restApi and the config is partially loading at that point but not complete. That's why is also added necessary null checks for the getFolders() and getDevice() part to catch a isConfigLoaded=true result while those functions called afterwards return null unexpectedly. We don't need much change to handle this gracefully. The fragments have timers so they'll retry to get the UI updated shortly if the race occured.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Synchronized functions is how you deal with concurrency and race conditions. It's not bad practice, the only other practice is using atomics (which I think in Java is close to impossible), or immutable objects which you always copy.

If you don't want to use synchronized, you need to synchronize around your lock every time you access mConfig in all other functions, as now it's a half way house, we lock in some cases but not the other.
If we lock in all cases this pretty much ends up being equivalent of just using public synchronized as that gets translated into synchronized(restApi) { restApi.getDevices() }. These locks are reentrant, and even if I didn't look at the code much, I can't imagine that any of these functions call out to a separate thread, and wait for it to respond, and the other thread tries to access the restApi object at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'll put the synchronized in tomorrow, then let's run the app for test and see if most things are running correctly as far as it can be seen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I've looked at the code again. To be "hands on" I'd suggest looking at RestApi#ignoreDevice. Just putting synchronized(mConfigLock) { ... } around the code in this function seems wrong to me as it calls RestApi#sendConfig conditionally. There, mConfig is read-accessed and sent via post (...). That means we would also have to add synchronized(mConfigLock) { ... } in sendConfig. This is the point where it is too complicated for me to do it now as "just adding it" would deadlock if the body of ignoreDevice holds the lock and sendConfig gets called while "locked".

Putting public synchronized void ignoreDevice() { ... } instead of the mConfigLock and doing this also on sendConfig() will not solve our problem. Every thread calling this RestApi#public functions will have its own lock, so one lock per function.

Can you give me a "hands on" example where to put a working lock, let's say, on two functions like the ignoreDevice and sendConfig (both called externally and one internally from the same class)? Seems I didn't get the point what exactly to do here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Synchronized locks are reentrant, so you can do synchronized(foo) { synchronized(foo) {} } as long as it happens on the same thread. Synchronized methods lock around the object, not the method.

You have plenty of other sections that access mConfigvariabke which also need to be under a lock.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh @AudriusButkevicius, I would really really appreciate it if you could at least overcome your directors position and one time do the extra work. Yeaaaah ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow I don't get rid of the bad feeling doing too much synchronize() adds in the code could break it and we don't see it coming. You might be technically right, but I feel better with granular improvements to the threading stuff I have to admin I don't have a complete oversight how "Nutomic" put all those threads and same-thread as well as foreign-thread handler, callback listener stuff. Problem is, I can put the improvement you require to merge this, but I cannot assure it won't negative impact. That's why I only started with one spot that got fixed.
@imsodin Maybe you can tell me if I'm right or wrong here. I can put the code Audrius' requested but I also don't want to break things going too far.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah no...
I quickly skimmed the conversation and I definitely can't give any relevant input. The first time I came in contact with parallel computing was on Syncthing in Go - I have no education on it, nor do I know java or the syncthing-android codebase. That's what I meant with high-level review in another place: I am happy to comment on concepts or interface (though I am no expert there either), but I can't in the technical division (and I don't see myself going there even long-term, so don't hold your breath ;) ).

I am aware this is disappointing, because I had very productive disagreements with Audrius (the amount of bulls*** that I would have inserted into Syncthing would have been astounding otherwise), but sometimes we did get stuck disagreeing - and then it's nice to get a tie-breaker from a third party. I just really can't be that party.

if (mConfig == null) {
throw new RuntimeException("config is null: " + result);
}
Expand Down Expand Up @@ -456,7 +463,11 @@ public void getSystemInfo(OnResultListener1<SystemInfo> listener) {
}

public boolean isConfigLoaded() {
return mConfig != null;
Boolean configLoaded;
synchronized(mConfigLock) {
configLoaded = mConfig != null;
}
return configLoaded;
}

/**
Expand Down