lib/config, cmd/syncthing: Better handle committing configuration (fixes #3077) #3386
Conversation
This file contains 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
#3077) This slightly changes the interface used for committing configuration changes. The two parts are now: - VerifyConfiguration, which runs synchronously and locked, and can abort the config change. These callbacks shouldn't *do* anything apart from looking at the config changes and saying yes or no. No change from previously. - CommitConfiguration, which runs asynchronously (one goroutine per call) *after* replacing the config and releasing any locks. Returning false from these methods sets the "requires restart" flag, which now lives in the config.Wrapper. This should be deadlock free as the CommitConfiguration calls can take as long as they like and can wait for locks to be released when they need to tweak things. I think this should be safe compared to before as the CommitConfiguration calls were always made from a random background goroutine (typically one from the HTTP server), so it was always concurrent with everything else anyway. Hence the CommitResponse type is gone, instead you get an error back on verification failure only, and need to explicitly check w.RequiresRestart() afterwards if you care. As an added bonus this fixes a bug where we would reset the "requires restart" indicator if a config that did not require restart was saved, even if we already were in the requires-restart state.
|
@st-jenkins retest this please |
|
@st-review merge |
st-review
pushed a commit
that referenced
this pull request
Jul 4, 2016
#3077) This slightly changes the interface used for committing configuration changes. The two parts are now: - VerifyConfiguration, which runs synchronously and locked, and can abort the config change. These callbacks shouldn't *do* anything apart from looking at the config changes and saying yes or no. No change from previously. - CommitConfiguration, which runs asynchronously (one goroutine per call) *after* replacing the config and releasing any locks. Returning false from these methods sets the "requires restart" flag, which now lives in the config.Wrapper. This should be deadlock free as the CommitConfiguration calls can take as long as they like and can wait for locks to be released when they need to tweak things. I think this should be safe compared to before as the CommitConfiguration calls were always made from a random background goroutine (typically one from the HTTP server), so it was always concurrent with everything else anyway. Hence the CommitResponse type is gone, instead you get an error back on verification failure only, and need to explicitly check w.RequiresRestart() afterwards if you care. As an added bonus this fixes a bug where we would reset the "requires restart" indicator if a config that did not require restart was saved, even if we already were in the requires-restart state. GitHub-Pull-Request: #3386
st-review
added
the
frozen-due-to-age
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Purpose
This slightly changes the interface used for committing configuration
changes. The two parts are now:
abort the config change. These callbacks shouldn't do anything
apart from looking at the config changes and saying yes or no. No
change from previously.
call) after replacing the config and releasing any locks. Returning
false from these methods sets the "requires restart" flag, which now
lives in the config.Wrapper.
This should be deadlock free as the CommitConfiguration calls can take
as long as they like and can wait for locks to be released when they
need to tweak things. I think this should be safe compared to before as
the CommitConfiguration calls were always made from a random background
goroutine (typically one from the HTTP server), so it was always
concurrent with everything else anyway.
Hence the CommitResponse type is gone, instead you get an error back on
verification failure only, and need to explicitly check w.RequiresRestart()
afterwards if you care.
As an added bonus this fixes a bug where we would reset the "requires
restart" indicator if a config that did not require restart was saved,
even if we already were in the requires-restart state.
Testing
I hammered on it a bit manually, and the tests that exist still pass.