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.

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

lib/fs: Watching is unsupported on android/amd64 (fixes #8709) #8710

Merged
merged 1 commit into from Dec 21, 2022

Conversation

calmh
Copy link
Member

@calmh calmh commented Dec 8, 2022

Apparently watching on android/amd64 causes a SIGSYS: bad syscall crash. This avoids that.

@Catfriend1
Copy link
Contributor

Thanks, @calmh , I'll try it on the Android 13 emulator and report back.

@Catfriend1 Catfriend1 self-requested a review December 8, 2022 11:01
@Catfriend1
Copy link
Contributor

Sorry, it's in german. I've taken this screenshot from the Google Play Console of Syncthing-Fork. It shows that x86_64 corresponds to a minority of devices. Syncthing-Fork had another workaround in place which worked for Android <= 11 since 2020 and therefore has ~ 100 "Chromebook" x86_64 users who are happy with it. Reading though the syncthing-android issue tracker yesterday, I saw some of them already voiced the request for the official Syncthing app to support x86_64. All in all, I consider the risk of doing something wrong here with the combination "x86_64" and "android" build is low - and we benefit in both projects that Syncthing will run on x86_64 devices where it just crashed before without the user being able to interact with the wrapper and its settings screens.

image

@Catfriend1
Copy link
Contributor

Catfriend1 commented Dec 8, 2022

SyncthingNative build log:
https://pastebin.com/W44bQMVE

-- all fine.

image

@Catfriend1
Copy link
Contributor

This PR works fine on an x86_64 Android. (AVD 13)

Screenshots:
image
image

@Catfriend1
Copy link
Contributor

Thanks for your help on this, Jakob 💯 👍 🚀

@imsodin
Copy link
Member

imsodin commented Dec 8, 2022

@Catfriend1 Could you give your view on this - especially the usability of the app without filesystem notifications and making users aware of that:

Am I somewhat unsure that this is the correct hammer: Are we sure this affects all android/amd64 systems? And also if it ever starts to work again, it means syncthing needs changing. It feels like disabling watching from the android wrapper on affected systems is preferrable, together with a warning to those users that there is no file watching - this means rescans every 1min on android, this will re-ignite the battery drain and everything so slow reports.

@Catfriend1
Copy link
Contributor

Catfriend1 commented Dec 8, 2022

@imsodin I'm not a big fan of the alternative because users might go to the web gui and try to enable file watching, ending in a crash with no possibility to start syncthing again and export/import config to recover. We would need a lot of code and xml handling in the wrapper to prevent the native running with default-enabled watches... code, even the fork doesn't have yet. Two years ago, the issue was reported to go/google trackers and the answer I've read always was "epollwait" won't come to amd64 android. The userbase of amd64-android is so small, in my opinion it's not worth to do much more coding work at the moment. Mainly Chromebook and emulator users (which is important to attract more people to help with the wrapper because they can use the emulator beyond Android 12). No one of these users was able to run the app (from my readings through issues) and it would be a good start, I think, to let them "in" even without the watcher functions. (Before we had file watching, Syncthing was already popular on Android :)

Doesn't the native automatically fall back to rescan 1 m if setting up those watches failed?

@imsodin
Copy link
Member

imsodin commented Dec 8, 2022

Thanks. That makes sense. I still think it would be good to have a warning on the app for those users, but that's obviously not a blocker to doing this fix to get it working at all.

Doesn't the native automatically fall back to rescan 1 m if setting up those watches failed?

As I wrote in the cited comment: It does, and that's what worries me, as it means a lot of fuse activity and thus likely high battery usage.

@Catfriend1
Copy link
Contributor

Do we need to modify the android app first?

@imsodin
Copy link
Member

imsodin commented Dec 16, 2022

I am fine with this PR. App changes for those few users affected would be nice but not a necessity in my opinion.

However I just found a link to an issue on fsnotify about the likely root cause - we should at least open a ticket on inotify about it: fsnotify/fsnotify#112
On phone right now thus just dropping the link here.

Never mind - skimming on the phone it looked promising but it's entirely unrelated to the problem at hand.

@calmh calmh merged commit ad0044f into syncthing:main Dec 21, 2022
calmh added a commit to calmh/syncthing that referenced this pull request Dec 21, 2022
* upstream/main:
  lib/fs: Watching is unsupported on android/amd64 (fixes syncthing#8709) (syncthing#8710)
  lib/model: Only log at info level if setting change time fails (syncthing#8725)
  lib/model: Don't lower rescan interval from default on auto accepted enc folder (fixes syncthing#8572) (syncthing#8573)
  gui, man, authors: Update docs, translations, and contributors
  gui: Remove unmaintained language variant nl-BE (syncthing#8722)
  gui, script: Fix indentation in lang-en.json to match others (syncthing#8721)
  docker: Ensure entrypoint is executable (syncthing#8719)
  Go 1.19.4
  Update dependencies (syncthing#8717)
  gui, man, authors: Update docs, translations, and contributors
@calmh calmh added this to the v1.22.3 milestone Dec 27, 2022
calmh added a commit to calmh/syncthing that referenced this pull request Jan 23, 2023
* main: (69 commits)
  Handle relay connect timeout (fixes syncthing#8749) (syncthing#8755)
  gui, man, authors: Update docs, translations, and contributors
  build: Go 1.19.5
  gui, man, authors: Update docs, translations, and contributors
  script: Add weblatedl.go for downloading updated translations (syncthing#8723)
  gui: Allow to translate action and type in Recent Changes modal (syncthing#8548)
  gui, man, authors: Update docs, translations, and contributors
  gui: Fix undefined lastSeenDays error in disconnected-inactive status check (ref syncthing#8530) (syncthing#8730)
  gui, man, authors: Update docs, translations, and contributors
  gui, api: Indicate running under container (syncthing#8728)
  lib/fs: Use io/fs errors as recommended in std lib (syncthing#8726)
  build: Handle co-authors (ref syncthing#3744) (syncthing#8708)
  lib/fs: Watching is unsupported on android/amd64 (fixes syncthing#8709) (syncthing#8710)
  lib/model: Only log at info level if setting change time fails (syncthing#8725)
  lib/model: Don't lower rescan interval from default on auto accepted enc folder (fixes syncthing#8572) (syncthing#8573)
  gui, man, authors: Update docs, translations, and contributors
  gui: Remove unmaintained language variant nl-BE (syncthing#8722)
  gui, script: Fix indentation in lang-en.json to match others (syncthing#8721)
  docker: Ensure entrypoint is executable (syncthing#8719)
  Go 1.19.4
  ...
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Dec 22, 2023
@syncthing syncthing locked and limited conversation to collaborators Dec 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants