Moritz Poldrack: 2
cmd: rename sync function to avoid collisions with stdlib package
cmd: make syncing run in parallel
2 files changed, 86 insertions(+), 78 deletions(-)
I like it! The one comment I have is we should change all of the log.Fatalf's in
the account goroutines into log.Errorf's + return. That way one account sync can
fail and not pull down the entire program. What do you think?
I think the os.Exit(1) in log.Fatal should be removed completely. Right
now a log.Fatal is basically worse than a panic since defers are not
run. So I think it should be part of another patchseries to make
cancellations and abortions (is that the correct word? sounds strange in
this context) less of a nuclear options and have them clean up behind
them (move already downloaded messages to cur, logout for all
connections, potentially other cleanup tasks…)
I agree - I actually (had) one that changed the calls to sync and diff to the
RunE method of cobra, and returned errors back up to the main loop, but that
becomes a moot point with this series to just return and exit the goroutine. We
can do that all as a separate patchset.
I'm fine with keeping syncRun in (1/2) too.
Reviewed-by: Tim Culverhouse <tim@timculverhouse.com>
--
Tim
cmd/sync.go | 160 +++++++++++++++++++++++++++-------------------------
1 file changed, 84 insertions(+), 76 deletions(-)
diff --git a/cmd/sync.go b/cmd/sync.go
index 353d94f..fbaa48c 100644
--- a/cmd/sync.go+++ b/cmd/sync.go
@@ -1,6 +1,7 @@
package main
import (
+ "sync" "sync/atomic"
"time"
@@ -33,96 +34,103 @@ func syncRun(cmd *cobra.Command, args []string) {
if err != nil {
log.Debugf("%v", err)
}
+ var wg sync.WaitGroup for _, cfg := range cfgs {
- log.Infof("offmap: syncing account %s", cfg.Name)- start := time.Now()+ wg.Add(1)+ go func(cfg *offmap.Config) {+ defer wg.Done()+ log.Infof("offmap: syncing account %s", cfg.Name)+ start := time.Now()- // Bootstrap the app- state, err := offmap.NewState(cfg)- if err != nil {- log.Fatalf("%v", err)- }- err = state.Load()- if err != nil {- log.Fatalf("%v", err)- }- local, err := maildir.NewStore(cfg)- if err != nil {- log.Fatalf("could not create store: %v", err)- }- remote, err := imap.NewStore(cfg)- if err != nil {- log.Fatalf("could not create store: %v", err)- }-- // Get the diffs- localDiff, err := local.Diff(state.Mailboxes)- if err != nil {- log.Fatalf("%v", err)- }- remoteDiff, err := remote.Diff(state.Mailboxes)- if err != nil {- log.Fatalf("%v", err)- }-- var quit atomic.Bool- go func() {- // Save our state if we cancel the command- <-cmd.Context().Done()- quit.Store(true)- }()-- // Do mailboxes first- err = remote.ApplyMailboxDiff(state, localDiff)- if err != nil {- // Save any changes we've made. log.Fatalf calls os.Exit(1).- // Defers are bypassed with this call- if err := state.Save(); err != nil {- log.Errorf("offmap: could not save state: %v", err)+ // Bootstrap the app+ state, err := offmap.NewState(cfg)+ if err != nil {+ log.Fatalf("%v", err) }
- log.Fatalf("imap: could not apply diff: %v", err)- }-- // Apply remote changes to local- err = local.ApplyMailboxDiff(state, remoteDiff)- if err != nil {- // Save any changes we've made. log.Fatalf calls os.Exit(1).- // Defers are bypassed with this call- if err := state.Save(); err != nil {- log.Errorf("offmap: could not save state: %v", err)+ err = state.Load()+ if err != nil {+ log.Fatalf("%v", err)+ }+ local, err := maildir.NewStore(cfg)+ if err != nil {+ log.Fatalf("could not create store: %v", err)+ }+ remote, err := imap.NewStore(cfg)+ if err != nil {+ log.Fatalf("could not create store: %v", err) }
- log.Fatalf("%v", err)- }- // Now the state has an up to date picture of mailboxes. Sync- // each mailbox, if it has changes- for _, mbox := range state.Mailboxes {- if quit.Load() {- break+ // Get the diffs+ localDiff, err := local.Diff(state.Mailboxes)+ if err != nil {+ log.Fatalf("%v", err)+ }+ remoteDiff, err := remote.Diff(state.Mailboxes)+ if err != nil {+ log.Fatalf("%v", err) }
- ok1, err := remote.ApplyEmailDiff(state, mbox.RemoteName, localDiff)++ var quit atomic.Bool+ go func() {+ // Save our state if we cancel the command+ <-cmd.Context().Done()+ quit.Store(true)+ }()++ // Do mailboxes first+ err = remote.ApplyMailboxDiff(state, localDiff) if err != nil {
- log.Errorf("%v", err)- continue+ // Save any changes we've made. log.Fatalf calls os.Exit(1).+ // Defers are bypassed with this call+ if err := state.Save(); err != nil {+ log.Errorf("offmap: could not save state: %v", err)+ }+ log.Fatalf("imap: could not apply diff: %v", err) }
- ok2, err := local.ApplyEmailDiff(state, mbox.LocalName, remoteDiff)++ // Apply remote changes to local+ err = local.ApplyMailboxDiff(state, remoteDiff) if err != nil {
- log.Errorf("%v", err)- continue+ // Save any changes we've made. log.Fatalf calls os.Exit(1).+ // Defers are bypassed with this call+ if err := state.Save(); err != nil {+ log.Errorf("offmap: could not save state: %v", err)+ }+ log.Fatalf("%v", err) }
- if ok1 || ok2 {- err = remote.UpdateMailboxStatus(state, mbox.LocalName)++ // Now the state has an up to date picture of mailboxes. Sync+ // each mailbox, if it has changes+ for _, mbox := range state.Mailboxes {+ if quit.Load() {+ break+ }+ ok1, err := remote.ApplyEmailDiff(state, mbox.RemoteName, localDiff) if err != nil {
log.Errorf("%v", err)
+ continue+ }+ ok2, err := local.ApplyEmailDiff(state, mbox.LocalName, remoteDiff)+ if err != nil {+ log.Errorf("%v", err)+ continue+ }+ if ok1 || ok2 {+ err = remote.UpdateMailboxStatus(state, mbox.LocalName)+ if err != nil {+ log.Errorf("%v", err)+ } }
}
- }- // Save and clean up- if err := state.Save(); err != nil {- log.Errorf("offmap: could not save state: %v", err)- }- remote.Cleanup()- log.Infof("offmap (%s): sync completed in %s", cfg.Name, time.Since(start).Round(1*time.Millisecond))+ // Save and clean up+ if err := state.Save(); err != nil {+ log.Errorf("offmap: could not save state: %v", err)+ }+ remote.Cleanup()+ log.Infof("offmap (%s): sync completed in %s", cfg.Name, time.Since(start).Round(1*time.Millisecond))+ }(cfg) }
++ wg.Wait()}
--
2.39.0