From 6cf51173d586fc8fb7fc1188d7a990f2ca835531 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 21 Oct 2026 18:03:58 +0200 Subject: [PATCH] Give the Service its own fetch client and open-document waits instead of reading package variables from background goroutines, which a test or reload changed meanwhile: data races under -race (issue 1601) --- modules/brother/cmd/brother/glass_test.go | 2 +- modules/brother/cmd/brother/main.go | 2 +- modules/brother/cmd/brother/scan.go | 42 ++++++++++++++++++----- 3 files changed, 36 insertions(+), 10 deletions(-) diff --git a/modules/brother/cmd/brother/glass_test.go b/modules/brother/cmd/brother/glass_test.go index ef89f713..6c5efcbc 100644 --- a/modules/brother/cmd/brother/glass_test.go +++ b/modules/brother/cmd/brother/glass_test.go @@ -264,7 +264,7 @@ func TestAnOpenDocumentIsFoundAgainAfterARestartAndItsPagesAreDeletedWhenClosed( docExpiry = 200 * time.Millisecond t.Cleanup(func() { docExpiry = old }) s := &Service{Settings: Settings{Scanner: f.base(), Folder: folder, Resolution: 200, ColorMode: "color"}, Now: func() time.Time { return fixedNow }, - Asks: askBus, AnswerWait: 5 * time.Second, ScanWait: 5 * time.Second, AnswerPoll: 5 * time.Millisecond, Jobs: NewJobs()} + Asks: askBus, AnswerWait: 5 * time.Second, ScanWait: 5 * time.Second, AnswerPoll: 5 * time.Millisecond, Jobs: NewJobs(), Fetch: fetchClient, DocExpiry: docExpiry} if err := s.Recover(context.Background()); err != nil { t.Fatal(err) } diff --git a/modules/brother/cmd/brother/main.go b/modules/brother/cmd/brother/main.go index 1b4209e9..34f0d577 100644 --- a/modules/brother/cmd/brother/main.go +++ b/modules/brother/cmd/brother/main.go @@ -112,7 +112,7 @@ func tools(settingsFile string, now func() time.Time) []stdio.Tool { } return &Service{Settings: s, Node: os.Getenv("MESH_NODE"), Now: now, Asks: askBus, AnswerWait: answerWait, ScanWait: scanWait, AnswerPoll: answerPoll, - QuickWait: quickWait, FeederWait: feederWait, Jobs: jobs, Upload: uploadVia, + QuickWait: quickWait, FeederWait: feederWait, Jobs: jobs, Upload: uploadVia, Fetch: fetchClient, DocExpiry: docExpiry, DocRetry: docRetry, Bucket: func() (string, error) { return ReadBucket(os.Getenv("BROTHER_BUCKET")) }}, nil } // ask is a tool that reads its arguments, and answers what the service gives. diff --git a/modules/brother/cmd/brother/scan.go b/modules/brother/cmd/brother/scan.go index e8ddf14b..a73faa9c 100644 --- a/modules/brother/cmd/brother/scan.go +++ b/modules/brother/cmd/brother/scan.go @@ -34,8 +34,13 @@ type Service struct { Jobs *Jobs // Upload asks a machine's node-files to send one of its files into the bucket and answers the key; Bucket is // brother's bucket. Both are the tool side's (printer.print with path and node). - Upload func(node, path, bucket string) (string, error) - Bucket func() (string, error) + // Fetch reads the links the device process signs; DocExpiry and DocRetry are the open documents' waits. They are + // copied into the Service when it is made, because background work reads them while a test (or a reload) changes the + // package's own, which would race. + Fetch *http.Client + DocExpiry, DocRetry time.Duration + Upload func(node, path, bucket string) (string, error) + Bucket func() (string, error) } // Jobs are the scans this tool process has started and not yet been asked the outcome of. A scan runs in the @@ -236,7 +241,7 @@ func (s *Service) finishScan(ctx context.Context, id string, wait time.Duration, } if doc, _ := ans["document"].(string); doc != "" && ans["key"] == nil { // A page was added to an open document: nothing to file yet, and the document is closed if it is left open. - s.armDocument(doc, docExpiry, 0) + s.armDocument(doc, s.docExpiry(), 0) return ans, nil } if ask.Document != "" && ask.Finish { @@ -296,7 +301,7 @@ func (s *Service) fileOne(ctx context.Context, ans map[string]any) (map[string]a var buf bytes.Buffer gctx, cancel := context.WithTimeout(ctx, 5*time.Minute) defer cancel() - if _, err := objectlink.Get(gctx, fetchClient, link, &buf, objectlink.MaxDocument, sum); err != nil { + if _, err := objectlink.Get(gctx, s.fetchClient(), link, &buf, objectlink.MaxDocument, sum); err != nil { return nil, fmt.Errorf("the scan is kept as %s, but it could not be read from the store: %w", key, err) } pdf := buf.Bytes() @@ -329,7 +334,28 @@ func withoutLink(ans map[string]any) map[string]any { return out } -// fetchClient reads the links the device process signs; a test puts a client of its own in its place. +func (s *Service) fetchClient() *http.Client { + if s.Fetch != nil { + return s.Fetch + } + return &http.Client{} +} + +func (s *Service) docExpiry() time.Duration { + if s.DocExpiry > 0 { + return s.DocExpiry + } + return 30 * time.Minute +} + +func (s *Service) docRetry() time.Duration { + if s.DocRetry > 0 { + return s.DocRetry + } + return 5 * time.Minute +} + +// fetchClientDefault is what a Service made by the tools reads the links with; a test puts a client of its own in its place before it makes the tools. var fetchClient = &http.Client{} // ScanStatus says whether the scanner answers, and what it is doing. A scanner that does not answer is an @@ -370,7 +396,7 @@ func (s *Service) armDocument(doc string, d time.Duration, attempts int) { if err == nil || attempts >= 10 || strings.Contains(err.Error(), "is closed already") || strings.Contains(err.Error(), "its id is wrong") { return } - s.armDocument(doc, docRetry, attempts+1) + s.armDocument(doc, s.docRetry(), attempts+1) }) } @@ -388,9 +414,9 @@ func (s *Service) Recover(ctx context.Context) error { if id == "" { continue } - left := docExpiry + left := s.docExpiry() if at, err := time.Parse(time.RFC3339, fmt.Sprint(m["updated"])); err == nil { - left = max(docExpiry-s.Now().Sub(at), time.Second) + left = max(s.docExpiry()-s.Now().Sub(at), time.Second) } s.armDocument(id, left, 0) }