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 race during config reload and missing null check (fixes #1194) #1195

Merged
merged 6 commits into from Jul 29, 2018

Conversation

Projects
None yet
3 participants
@Catfriend1
Copy link
Contributor

Catfriend1 commented Jul 26, 2018

Purpose
Fix race during config reload and missing null check in FolderListFragment and DeviceListFragment
See related issue: #1194

Testing
Verified working at commit 6a8cc7a (device lg-h815, Android 7.1.2)

Fix race during config reload and missing null check
in FolderListFragment and DeviceListFragment (fixes #1194)

@Catfriend1 Catfriend1 added the bug label Jul 26, 2018

@Catfriend1 Catfriend1 self-assigned this Jul 26, 2018

@Catfriend1 Catfriend1 requested a review from AudriusButkevicius Jul 26, 2018

@Catfriend1 Catfriend1 added this to the 0.10.13 milestone Jul 26, 2018

mConfig = new Gson().fromJson(result, Config.class);
synchronized(mConfigLock) {
mConfig = new Gson().fromJson(result, Config.class);
}

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 26, 2018

Member

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.

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 26, 2018

Author Contributor

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

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 26, 2018

Member

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.

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 26, 2018

Author Contributor

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.

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 27, 2018

Author Contributor

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.

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 27, 2018

Member

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.

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 29, 2018

Author Contributor

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

This comment has been minimized.

@Catfriend1

Catfriend1 Jul 29, 2018

Author Contributor

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.

This comment has been minimized.

@imsodin

imsodin Jul 30, 2018

Member

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.

Catfriend1 added some commits Jul 26, 2018

@Catfriend1 Catfriend1 removed this from the 0.10.13 milestone Jul 28, 2018

@Catfriend1 Catfriend1 requested a review from imsodin Jul 29, 2018

while (it.hasNext()) {
Folder f = it.next();
if (f.id.equals(id)) {
it.remove();

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 29, 2018

Member

This is not related to this change, and feel free to ignore this, but just noticed that we're missing a break here.

while (it.hasNext()) {
Device d = it.next();
if (d.deviceID.equals(deviceId)) {
it.remove();

This comment has been minimized.

@AudriusButkevicius

AudriusButkevicius Jul 29, 2018

Member

Ditto about breaking.

@AudriusButkevicius
Copy link
Member

AudriusButkevicius left a comment

Looks good to me, feel free to merge.

@Catfriend1 Catfriend1 merged commit b50fcf1 into syncthing:master Jul 29, 2018

2 checks passed

Build (Syncthing Android) TeamCity build finished
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Catfriend1 Catfriend1 deleted the Catfriend1:fixNPE-DeviceListFragment branch Jul 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment