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

panic: send on closed channel #6815

Closed
TRSx80 opened this issue Jul 7, 2020 · 11 comments
Closed

panic: send on closed channel #6815

TRSx80 opened this issue Jul 7, 2020 · 11 comments
Labels
bug A problem with current functionality, as opposed to missing functionality (enhancement) frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion

Comments

@TRSx80
Copy link

TRSx80 commented Jul 7, 2020

Good day. I have been Syncthing user for some years now, and never so much as a peep of trouble! Therefore I never had occasion to thank all of you for all of your work, and perhaps even more importantly, sharing it like civilized people. So, cheers! 🍻

Because I am using syncthing-android fork, I originally had lodged this issue over there, however Catfriend1 seems to have confirmed that this appears to be an issue in core and not his wrapper.

I copy/paste some of requested information from that thread here:

Description of the issue

Upon starting up the wrapper, the main Syncthing process seems to crash after only a few seconds.

Version Information

App Version: 1.6.0.4
Syncthing Version: not sure how to find out?
Android Version: LineageOS 12.1 (Android 5.1.1)
Device manufacturer: Samsung
Device model: Galaxy s5

Device platform info

[ro.product.board]: [MSM8974]
[ro.product.brand]: [samsung]
[ro.product.cpu.abi2]: [armeabi]
[ro.product.cpu.abi]: [armeabi-v7a]
[ro.product.cpu.abilist32]: [armeabi-v7a,armeabi]
[ro.product.cpu.abilist64]: []
[ro.product.cpu.abilist]: [armeabi-v7a,armeabi]
[ro.product.device]: [kltetmo]
[ro.product.locale.language]: [en]
[ro.product.locale.region]: [US]
[ro.product.manufacturer]: [samsung]
[ro.product.model]: [SM-G900T]
[ro.product.name]: [cm_klte]

I did a little research into this, it looks to me like perhaps there may have been some possibly related issues however after reading a bit I very quickly realized I was in over my head and I should probably just post a new issue instead. Many of those look unresolved to me, frozen due to age, etc. but perhaps this has been fixed in the meantime, and/or someone more familiar with the codebase could refer me somewhere more relevant.

If above is not the case, I suppose you will want full logs for further troubleshooting, so let me work on sanitizing PII out of those and then I will try to find some place to upload them and share a link here. There are also perhaps relevant clues in that other thread, but I won't repeat those here (for now).

@TRSx80 TRSx80 added bug A problem with current functionality, as opposed to missing functionality (enhancement) needs-triage New issues needed to be validated labels Jul 7, 2020
@AudriusButkevicius
Copy link
Member

You haven't provided any logs, so there is no indication where this is happening.

@calmh
Copy link
Member

calmh commented Jul 7, 2020

Plenty of candidates in sentry though.

https://sentry.syncthing.net/syncthing/syncthing/issues/2127/ looks like it might still be a thing

@calmh calmh removed the needs-triage New issues needed to be validated label Jul 7, 2020
@TRSx80
Copy link
Author

TRSx80 commented Jul 7, 2020

Thanks guys for the immediate response.

I already have logcats in text files locally, but there is absolutely tons of PII in there. On one hand I try and contribute as much as possible bug reports (and other resources) to F/LOSS, and on the other I am also privacy conscious person. So this seems to be a recurring issue for me. I had the idea a while back to make a simple sanitization script with sed which will read in some file with x,y and substitute any instance of x with y, then I have a simple text file I can just keep adding x,y as needed whenever I realize there is some other piece of PII I missed before. So I need to finish that first. In fact I am working on it right now. Sorry for offtopic idiotic ramblings, but this is something I planned to do for a while now anyway, so I guess this will be the push I need to finish that.

In the meantime, I posted partial logs at the other linked issue. But I imagine there is not enough there to be helpful.

@imsodin
Copy link
Member

imsodin commented Jul 7, 2020

A stacktrace has just references to already publicly known source code, no personal information in there (except if you built yourself, the path to your source might be personal info, but it doesn't get much more straightforward than getting rid of that). Or you could just follow the link @calmh posted, look at a stacktrace and report if it's equivalent to what you got or not.

This issue has never been not a thing, except that it's impossible - well clearly it's not, but I looked so many times at it and keep failing to see how it can happen...
The scanChan channel is stored in a local variable in pull and is closed by defer. When pull returns, so has pullIteration. And pullIteration has no early returns: It waits for all the routines to stop before returning. The only other place where we spawn goroutines is in pullerRoutine, and it too waits for all it's spawned routines to stop.

@TRSx80
Copy link
Author

TRSx80 commented Jul 7, 2020

A stacktrace has just references to already publicly known source code, no personal information in there

This is logcat on Android, there is tons of PII in here. So far I found Wi-Fi station IDs, GPS coordinates, user names, public keys, etc. and I'm not even 1/3 through the file yet.

Or should I be compiling logs from some other source?

@AudriusButkevicius
Copy link
Member

This is an interesting one.
Having checked the code I agree this should be impossible, yet clearly it happens.
I wonder if there is something weird with deferred firing on a separate go routine, not the routine that is unwinding it's stack?

@TRSx80
Copy link
Author

TRSx80 commented Jul 8, 2020

Well, I just spent the better part of today brushing up my sed and regex skills. 😄

I think/hope I managed to remove most if not all PII. Feedback welcome if anyone notices anything (I really don't know much about Android internals, so I made some guesses here and there).

Also, I always knew there was a lot going on all the time in Android, but even knowing that, getting a really close look at those logs was still a real eye opener for me today. I think in the future I will just take a gander at logcat periodically, just to make sure nothing "funny" is going on. I already uninstalled some things immediately after today's exercise. Real GNU/Linux phones can not get here fast enough, I am so disgusted with Android/Google at this point.

Anyway, here are the logs. Debian paste only allow 150kB per paste, so I had to split it up into four pastes:

  1. https://paste.debian.net/hidden/ac27117d
  2. https://paste.debian.net/hidden/9152f0ca
  3. https://paste.debian.net/hidden/472e1930
  4. https://paste.debian.net/hidden/c8955f13

I have set these pastes to expire after 90 days, I figured that should be sufficient.

@TRSx80
Copy link
Author

TRSx80 commented Jul 10, 2020

I received update today of syncthing-android (fork) via F-Droid, and when it restarted Syncthing, it stayed up without crashing and has been working ever since.

I cannot explain this behaviour, but I thought I had better report it anyway. I also reported it in my original issue over at syncthing-android fork.

EDIT: Almost forgot, I now have one device stuck "Syncing" at 49% with some folders "Out of Sync" on my Android. I imagine this is some side effect of all the earlier crashes (perhaps corrupting the database, or?). I do not know enough about Syncthing internals, so any advice how to proceed on that would be appreciated.

@TRSx80
Copy link
Author

TRSx80 commented Jul 10, 2020

Somehow I managed to overlook the fact that this latest release of syncthing-android (fork) also has bumped the core version from 1.6.0 to 1.7.0! So maybe something got fixed in there in the meantime?

@TRSx80
Copy link
Author

TRSx80 commented Jul 11, 2020

one device stuck "Syncing" at 49% with some folders "Out of Sync" on my Android

Remaining stuck bits revolved around a few apparently fairly run of the mill issues, which were easily solved with a little searching:

  • file modified but not rescanned; will try again later (case only renames)
  • illegal characters
  • some permissions issues
  • etc...

So, very happy to report, everything appears fully back working now! 🍻

Original (and main) issue, well I will leave that up to you guys what you want to do. I am not sure if 1.7.0 really solved it or what happened. But, so far, so good... 🤞

@imsodin
Copy link
Member

imsodin commented Apr 20, 2021

Closing as I don't see any send on closed channel events anymore in sentry.

@imsodin imsodin closed this as completed Apr 20, 2021
@calmh calmh added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Aug 11, 2022
@syncthing syncthing locked and limited conversation to collaborators Aug 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug A problem with current functionality, as opposed to missing functionality (enhancement) frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

No branches or pull requests

4 participants