lib/model: Remove runner during folder cleanup (fixes #9269) #9271
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
Well, good to see that we independently reached the same conclusion about the problem at least. Though we now have paralell issues and PRs :p |
Member
Author
|
Was about to check your PR, already closed "my" ticket again in favor of yours |
calmh
approved these changes
Dec 7, 2023
Member
calmh
left a comment
There was a problem hiding this comment.
Cool. I think I prefer your solution. We should add a fixes for my new issue as well.
lib/model/service_map.go
Outdated
| return | ||
| } | ||
|
|
||
| // RemoveAndWaitChan removes the service at the given key, stopping it on |
Member
There was a problem hiding this comment.
Suggested change
| // RemoveAndWaitChan removes the service at the given key, stopping it on | |
| // StopAndWaitChan removes the service at the given key, stopping it on |
Member
Author
After looking at your PR I think there's still a tiny bug in mine: Removing the token from the service map happens under read-lock, so multiple goroutines might race for removing it. I dont think it can cause any problems, but it's not right. I'll also remove it in |
Member
Author
|
Luckily stopping a service in suture is idempotent. The bool return on Except of course I haven't pushed yet, soon :P |
Before introducing the service map and using it for folder runners, the entries in folderCfgs and folderRunners for the same key/folder were removed under a single lock. Stopping the folder happens separately before that with just the read lock. Now with the service map stopping the folder and removing it from the map is a single operation. And that still happens with just a read-lock. However even with a full lock it’s still problematic: After the folder stopped, the runner isn’t present anymore while the folder-config still is and sais the folder isn't paused. The index handler in turn looks at the folder config that is not paused, thus assumes the runner has to be present -> nil deref on the runner. A better solution would like be to push most of these fmut maps into the folder - they anyway are needed in there. Then there's just a single map/source of info that's necessarily consistent.
87b724d to
bb5cd15
Compare
calmh
added a commit
that referenced
this pull request
Dec 11, 2023
This is no longer a notable condition, as we do this pretty much all the time.
calmh
added a commit
that referenced
this pull request
Dec 11, 2023
This is no longer a notable condition, as we do this pretty much all the time.
calmh
added a commit
to cjc7373/syncthing
that referenced
this pull request
Dec 11, 2023
* main: (69 commits) lib/nat: Fix test build failure (ref syncthing#9010) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010) gui, man, authors: Update docs, translations, and contributors gui: Show folder/device status on small screens (syncthing#8643) lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271) build: Update dependencies (syncthing#9265) build: Revert specifics for Go 1.21.4, build using Go 1.21.5 (syncthing#9264) lib/fs: Reduce memory usage in xattrs handling (syncthing#9251) lib/model: Improve LastSeen handling (syncthing#9256) lib/scanner: Record inode change time for directories and symlinks (syncthing#9250) lib/api: Improve ignore loading error handling (fixes syncthing#9253) (syncthing#9254) gui, man, authors: Update docs, translations, and contributors lib/fs: Better equality comparison in mtimefs cmd/stcrashreceiver: Add metrics for diskstore inventory cmd/stcrashreceiver: Minor cleanup, stricter file permissions cmd/stcrashreceiver: Add metrics for incoming reports ...
calmh
added a commit
to calmh/syncthing
that referenced
this pull request
Dec 16, 2023
* main: (89 commits) build: Update quic-go (fixes syncthing#9287) lib/model: Only handle relevant folder summaries (kqueue) (fixes syncthing#9183) (syncthing#9288) lib/model: Use a single lock (phase two: cleanup) (syncthing#9276) build(deps): bump actions/setup-go from 4 to 5 (syncthing#9279) lib/model: Use a single lock (syncthing#9275) cmd/syncthing: Better cli stdin handling (ref syncthing#9166) (syncthing#9281) cmd/syncthing: Mostly replace urfave/cli command line parser with alecthomas/kong (syncthing#9166) lib/nat: Fix test build failure (ref syncthing#9010) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010) gui, man, authors: Update docs, translations, and contributors gui: Show folder/device status on small screens (syncthing#8643) lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271) build: Update dependencies (syncthing#9265) build: Revert specifics for Go 1.21.4, build using Go 1.21.5 (syncthing#9264) lib/fs: Reduce memory usage in xattrs handling (syncthing#9251) lib/model: Improve LastSeen handling (syncthing#9256) ...
calmh
added a commit
to calmh/syncthing
that referenced
this pull request
Jan 14, 2024
* main: lib/model: Use a single lock (phase two: cleanup) (syncthing#9276) build(deps): bump actions/setup-go from 4 to 5 (syncthing#9279) lib/model: Use a single lock (syncthing#9275) cmd/syncthing: Better cli stdin handling (ref syncthing#9166) (syncthing#9281) cmd/syncthing: Mostly replace urfave/cli command line parser with alecthomas/kong (syncthing#9166) lib/nat: Fix test build failure (ref syncthing#9010) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010) gui, man, authors: Update docs, translations, and contributors gui: Show folder/device status on small screens (syncthing#8643) lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before introducing the service map and using it for folder runners, the entries in folderCfgs and folderRunners for the same key/folder were removed under a single lock. Stopping the folder happens separately before that with just the read lock. Now with the service map stopping the folder and removing it from the map is a single operation. And that still happens with just a read-lock. However even with a full lock it’s still problematic: After the folder stopped, the runner isn’t present anymore while the folder-config still is and sais the folder isn't paused.
The index handler in turn looks at the folder config that is not paused, thus assumes the runner has to be present -> nil deref on the runner.
A better solution might be to push most of these fmut maps into the folder - they anyway are needed in there. Then there's just a single map/source of info that's necessarily consistent. That's quite a bit of work though, and probably/likely there will be corner cases there too.