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/model: Return paused summary instead of error on paused folders #6272

Merged
merged 1 commit into from Jan 17, 2020

Conversation

@imsodin
Copy link
Member

imsodin commented Jan 16, 2020

See https://forum.syncthing.net/t/rest-db-status-returns-404-on-startup/14329

If the api starts while the folder is not yet running or generally when querying /rest/db/status for a an about to be started folder, we return a 404. Now it is treated the same as a paused folder, i.e. send a valid folder summary. However the comment was wrong: We can't get any info from db for a paused folder, nevertheless it's important to get valid entries with zero values, otherwise the UI blows up.

@calmh

This comment has been minimized.

Copy link
Member

calmh commented Jan 17, 2020

This will leave some other attributes in odd states though, like the "State" field which will become entirely blank and a 0001-01-01 state changed date because there is no runner and

syncthing/lib/model/model.go

Lines 2304 to 2316 in e782bab

func (m *model) State(folder string) (string, time.Time, error) {
m.fmut.RLock()
runner, ok := m.folderRunners[folder]
m.fmut.RUnlock()
if !ok {
// The returned error should be an actual folder error, so returning
// errors.New("does not exist") or similar here would be
// inappropriate.
return "", time.Time{}, nil
}
state, changed, err := runner.getState()
return state.String(), changed, err
}

@imsodin

This comment has been minimized.

Copy link
Member Author

imsodin commented Jan 17, 2020

That's the same thing that happens today when the folder is paused. I agree it's not ideal, but it was the most minimal and consistent (aka leave the API behaviour mostly untouched) fix I came up with. One alternative I thought about was setting the state field to paused: That's inconsistent with it's previous use, where it is about the state a running folder is in. Also is it state, or error and we don't have a stateChanged time in either case. Another introducing another boolean field running and when the folder is not running, send a summary with just that field set to false. That however breaks our web UI by not sending zero values for the local/global item fields. It's definitely an API change and would require tweaking the UI or defining a minimal empty summary, that gets sent along even if !running. (Or even more radical: Don't remove paused folders from the model, only stop them, then the folder summary service still has access to db and can correctly report the state at the time of pausing).

@calmh

This comment has been minimized.

Copy link
Member

calmh commented Jan 17, 2020

Oh, right, already happens... Huh. This is not ideal imho, but a separate issue then.

@calmh calmh merged commit 39891cd into syncthing:master Jan 17, 2020
12 checks passed
12 checks passed
Build (Debian, Snap) (Syncthing) TeamCity build finished
Details
Build (Discovery Server) TeamCity build finished
Details
Build (Linux, Cross) (Syncthing) TeamCity build finished
Details
Build (Linux, Go1.12) (Syncthing) TeamCity build finished
Details
Build (Linux, Go1.14-rc) (Syncthing) TeamCity build finished
Details
Build (Mac) (Syncthing) TeamCity build finished
Details
Build (Relay Server) TeamCity build finished
Details
Build (Source) (Syncthing) TeamCity build finished
Details
Build (Windows) (Syncthing) TeamCity build finished
Details
Check Correctness (Syncthing) TeamCity build finished
Details
Coverage (Syncthing) TeamCity build finished
Details
GolangCI No issues found!
Details
@imsodin imsodin deleted the imsodin:model/summaryPausedErr branch Jan 17, 2020
calmh added a commit to calmh/syncthing that referenced this pull request Jan 21, 2020
* master:
  lib/model: Handle progress emitter zero interval (fixes syncthing#6281) (syncthing#6282)
  build(deps): bump github.com/pkg/errors from 0.9.0 to 0.9.1 (syncthing#6279)
  cmd/syncthing: Always use monitor process (fixes syncthing#4774, fixes syncthing#5786) (syncthing#6278)
  lib/syncthing: Wait for actual termination on Stop() (syncthing#6277)
  lib/model: Remove legacy handling of symlinks (syncthing#6276)
  lib/model: Return paused summary instead of error on paused folders (syncthing#6272)
  lib/config: Add some info to the folder marker missing (ref syncthing#5207) (syncthing#6270)
@calmh calmh added this to the v1.4.0 milestone Jan 24, 2020
calmh added a commit to calmh/syncthing that referenced this pull request Jan 30, 2020
* master: (28 commits)
  gui, man, authors: Update docs, translations, and contributors
  lib/config, lib/model: Limit concurrent pulls (fixes syncthing#5914) (syncthing#6290)
  lib/db: Fixup last commit with better key name
  lib/db: Configurable block GC time (syncthing#6295)
  lib/db: Deduplicate block lists in database (fixes syncthing#5898) (syncthing#6283)
  lib/relays: Fix incorrect timeout, bring back logging (ref syncthing#6289) (syncthing#6291)
  gui, man, authors: Update docs, translations, and contributors
  all: Transactionalize db.FileSet (fixes syncthing#5952) (syncthing#6239)
  lib/model: Handle progress emitter zero interval (fixes syncthing#6281) (syncthing#6282)
  build(deps): bump github.com/pkg/errors from 0.9.0 to 0.9.1 (syncthing#6279)
  cmd/syncthing: Always use monitor process (fixes syncthing#4774, fixes syncthing#5786) (syncthing#6278)
  lib/syncthing: Wait for actual termination on Stop() (syncthing#6277)
  lib/model: Remove legacy handling of symlinks (syncthing#6276)
  lib/model: Return paused summary instead of error on paused folders (syncthing#6272)
  lib/config: Add some info to the folder marker missing (ref syncthing#5207) (syncthing#6270)
  assets, gui: Losslessly compress all JPG, PNG, and PDF images (syncthing#6265)
  cmd/strelaypoolsrv: Serve gzip compressed responses
  gui, man, authors: Update docs, translations, and contributors
  gui, man, authors: Update docs, translations, and contributors
  build: go mod tidy
  ...
calmh added a commit to calmh/syncthing that referenced this pull request Jan 30, 2020
* master: (32 commits)
  gui, man, authors: Update docs, translations, and contributors
  lib/config, lib/model: Limit concurrent pulls (fixes syncthing#5914) (syncthing#6290)
  lib/db: Fixup last commit with better key name
  lib/db: Configurable block GC time (syncthing#6295)
  lib/db: Deduplicate block lists in database (fixes syncthing#5898) (syncthing#6283)
  lib/relays: Fix incorrect timeout, bring back logging (ref syncthing#6289) (syncthing#6291)
  gui, man, authors: Update docs, translations, and contributors
  all: Transactionalize db.FileSet (fixes syncthing#5952) (syncthing#6239)
  lib/model: Handle progress emitter zero interval (fixes syncthing#6281) (syncthing#6282)
  build(deps): bump github.com/pkg/errors from 0.9.0 to 0.9.1 (syncthing#6279)
  cmd/syncthing: Always use monitor process (fixes syncthing#4774, fixes syncthing#5786) (syncthing#6278)
  lib/syncthing: Wait for actual termination on Stop() (syncthing#6277)
  lib/model: Remove legacy handling of symlinks (syncthing#6276)
  lib/model: Return paused summary instead of error on paused folders (syncthing#6272)
  lib/config: Add some info to the folder marker missing (ref syncthing#5207) (syncthing#6270)
  assets, gui: Losslessly compress all JPG, PNG, and PDF images (syncthing#6265)
  cmd/strelaypoolsrv: Serve gzip compressed responses
  gui, man, authors: Update docs, translations, and contributors
  gui, man, authors: Update docs, translations, and contributors
  build: go mod tidy
  ...
imsodin added a commit to imsodin/syncthing that referenced this pull request Feb 11, 2020
imsodin added a commit to imsodin/syncthing that referenced this pull request Feb 11, 2020
…, ref syncthing#6272)
imsodin added a commit to imsodin/syncthing that referenced this pull request Feb 11, 2020
imsodin added a commit to imsodin/syncthing that referenced this pull request Feb 11, 2020
calmh pushed a commit that referenced this pull request Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.