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

Shutdown takes too long, triggering fmut deadlock panic #5781

Closed
syncthing-sentry bot opened this issue Jun 11, 2019 · 11 comments

Comments

Projects
None yet
3 participants
@syncthing-sentry
Copy link

commented Jun 11, 2019

When the Syncthing is shut down and thus the database closed, iterations may still be happening (e.g. receiving indexes). This is not safe according to the goleveldb documentation. And in general it isn't nice if Syncthing just doesn't shut down for a long while after the user asked it to do so. And this triggers the deadlock detector in RCs.

Sentry Issue: SYNCTHING-G

@imsodin imsodin changed the title Shutdown takes too long, triggering fmut deadlock panic. Shutdown takes too long, triggering fmut deadlock panic Jun 11, 2019

@imsodin imsodin self-assigned this Jun 11, 2019

@imsodin imsodin added the bug label Jun 11, 2019

imsodin added a commit to imsodin/syncthing that referenced this issue Jun 11, 2019

cmd/syncthing, lib/db: Exit/close db faster (fixes syncthing#5781)
This adds a 20s timeout on closing the db and additionally cancels active
db iterators and waits for them to terminate before closing the db.

imsodin added a commit to imsodin/syncthing that referenced this issue Jun 11, 2019

cmd/syncthing, lib/db: Exit/close db faster (fixes syncthing#5781)
This adds a 20s timeout on closing the db and additionally cancels active
db iterators and waits for them to terminate before closing the db.

imsodin added a commit to imsodin/syncthing that referenced this issue Jun 11, 2019

cmd/syncthing, lib/db: Exit/close db faster (fixes syncthing#5781)
This adds a 10s timeout on closing the db and additionally cancels active
db iterators and waits for them to terminate before closing the db.
@calmh

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

This is not by itself a great issue as the general public cannot follow the sentry link...

@imsodin

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Yeah, would have been nice if opening an issue would be just one click on sentry - I'll edit the OP.

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Is it not one click? We can make the org public I guess, so others can read it.

@imsodin

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Actually I am talking bullshit: You can edit the issue description in the sentry interface, so all good.

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Should we make sentry public?

If yes, raise an issue and assign to me not to forget.

@calmh

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

I looked around for five minutes and didn't see how to do that, but if we can that would be nice. Or just allow signup for any and all passers by.

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Re this ticker, deadlock detector is debug builds only no? Surely it's better to hang till it's ok than to end up with a screwed database?

Slow drive to fsync?

@calmh

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Deadlock detector didn't do anything here other than alert us to the fact that Close() had been blocked for 20 minutes because some iterator was clearing out the index as part of an incoming initial index exchange.

I don't know why you guys are super worried about screwing up the database when we've exited without close for literally five years. Syncthing is supposed to survive a sudden stop.

@AudriusButkevicius

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

🤷‍♂ sure whatever, but the ideal solution is clean shutdown. If that means bastardizing the iterators, so be it.

@imsodin

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

I don't know why you guys are super worried about screwing up the database when we've exited without close for literally five years. Syncthing is supposed to survive a sudden stop.

I am not super worried, it's just my personal expectation: Shut down as cleanly as possible. If that takes too long, whatever is controlling the process should somehow notify me that an "application is taking ages to shutdown", at which point I do get mildly annoyed and force close it. As you stated we have good reason to assume that shutting down before closing the db is not particularly bad, so I think it's a good compromise to automatically force close with a notification after some time (10s now).

@calmh

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

I agree. A time limited close is fine by me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.