From 37ddd00c3857640eea68e22dc51164ea2bf96e9b Mon Sep 17 00:00:00 2001 From: Alex Goodman Date: Fri, 2 Oct 2026 11:20:36 -0400 Subject: [PATCH] identify_license: fix send on closed channel in v2 backend `ClassifyLicenses` closes the `task` channel once the wait group drains, but each worker's deferred cleanup calls `wg.Done()` before `task <- true`. The closer goroutine can run between the two, and the final send panics with `send on closed channel`. Nothing ranges over `task`, so it is now left open. #21 fixed the same panic in the v1 backend, but the v2 copy still had it. Also fixed in the same backend: - `ClassifyLicensesWithContext` returned as soon as ctx was done but left the classification running in the background, racing on the named return, leaking the goroutine, and appending results into later calls on the same backend. ctx is now checked in the dispatch loop: once it is done no new files start, in-flight files finish, and the call returns `ctx.Err()`. Nothing outlives the call, though it can return slightly after the deadline. - `GetResults` read `b.results` without the lock. It now reads under the lock and returns a copy. Signed-off-by: Alex Goodman --- v2/tools/identify_license/backend/backend.go | 60 +++++++----- .../identify_license/backend/backend_test.go | 97 +++++++++++++++++++ 2 files changed, 132 insertions(+), 25 deletions(-) create mode 100644 v2/tools/identify_license/backend/backend_test.go diff --git a/v2/tools/identify_license/backend/backend.go b/v2/tools/identify_license/backend/backend.go index 1a98207..7b3983f 100644 --- a/v2/tools/identify_license/backend/backend.go +++ b/v2/tools/identify_license/backend/backend.go @@ -69,7 +69,19 @@ func (b *ClassifierBackend) SetTraceConfiguration(tc *classifier.TraceConfigurat } // ClassifyLicenses runs the license classifier over the given file. -func (b *ClassifierBackend) ClassifyLicenses(numTasks int, filenames []string, headers bool) (errors []error) { +func (b *ClassifierBackend) ClassifyLicenses(numTasks int, filenames []string, headers bool) []error { + return b.classifyLicenses(context.Background(), numTasks, filenames, headers) +} + +// ClassifyLicensesWithContext runs the license classifier over the given file. +// Once ctx is done no further files are started, and the call returns ctx.Err() +// after the files already in flight finish, so it can return slightly after the +// deadline. No classification work outlives the call. +func (b *ClassifierBackend) ClassifyLicensesWithContext(ctx context.Context, numTasks int, filenames []string, headers bool) []error { + return b.classifyLicenses(ctx, numTasks, filenames, headers) +} + +func (b *ClassifierBackend) classifyLicenses(ctx context.Context, numTasks int, filenames []string, headers bool) (errors []error) { // Create a pool from which tasks can later be started. We use a pool because the OS limits // the number of files that can be open at any one time. task := make(chan bool, numTasks) @@ -83,6 +95,8 @@ func (b *ClassifierBackend) ClassifyLicenses(numTasks int, filenames []string, h analyze := func(filename string) { defer func() { wg.Done() + // task is never closed, nothing ranges over it and this send runs + // after wg.Done(), so closing it would panic with "send on closed channel". task <- true }() if err := b.classifyLicense(filename, headers); err != nil { @@ -90,40 +104,32 @@ func (b *ClassifierBackend) ClassifyLicenses(numTasks int, filenames []string, h } } +dispatch: for _, filename := range filenames { + // checked first so a done ctx always wins over a free task slot. + if ctx.Err() != nil { + break + } + select { + case <-ctx.Done(): + break dispatch + case <-task: + } wg.Add(1) - <-task go analyze(filename) } - go func() { - wg.Wait() - close(task) - close(errs) - }() + wg.Wait() + close(errs) + if err := ctx.Err(); err != nil { + return []error{err} + } for err := range errs { errors = append(errors, err) } return errors } -// ClassifyLicensesWithContext runs the license classifier over the given file; ensure that it will respect the timeout in the provided context. -func (b *ClassifierBackend) ClassifyLicensesWithContext(ctx context.Context, numTasks int, filenames []string, headers bool) (errors []error) { - done := make(chan bool) - go func() { - errors = b.ClassifyLicenses(numTasks, filenames, headers) - done <- true - }() - select { - case <-ctx.Done(): - err := ctx.Err() - errors = append(errors, err) - return errors - case <-done: - return errors - } -} - // classifyLicense is called by a Go-function to perform the actual // classification of a license. func (b *ClassifierBackend) classifyLicense(filename string, headers bool) error { @@ -162,5 +168,9 @@ func (b *ClassifierBackend) classifyLicense(filename string, headers bool) error // GetResults returns the results of the classifications. func (b *ClassifierBackend) GetResults() results.LicenseTypes { - return b.results + // read under the lock and return a copy, so callers can sort or hold the + // results while another classification on this backend is appending. + b.mu.Lock() + defer b.mu.Unlock() + return append(results.LicenseTypes(nil), b.results...) } diff --git a/v2/tools/identify_license/backend/backend_test.go b/v2/tools/identify_license/backend/backend_test.go new file mode 100644 index 0000000..2cb4777 --- /dev/null +++ b/v2/tools/identify_license/backend/backend_test.go @@ -0,0 +1,97 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package backend + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/google/licenseclassifier/v2/assets" +) + +func writeLicense(t *testing.T) string { + t.Helper() + b, err := assets.ReadLicenseFile("License/MIT/pristine.txt") + if err != nil { + t.Fatal(err) + } + path := filepath.Join(t.TempDir(), "LICENSE") + if err := os.WriteFile(path, b, 0o600); err != nil { + t.Fatal(err) + } + return path +} + +// TestClassifyLicensesRepeated calls ClassifyLicenses many times on one file. +// Closing the task channel used to race with the last worker returning its +// slot, which panicked with "send on closed channel". Run with -race for the +// most reliable reproduction. +func TestClassifyLicensesRepeated(t *testing.T) { + be, err := New() + if err != nil { + t.Fatal(err) + } + path := writeLicense(t) + + for i := 0; i < 200; i++ { + if errs := be.ClassifyLicenses(10, []string{path}, false); errs != nil { + t.Fatalf("ClassifyLicenses() = %v", errs) + } + } + + found := false + for _, r := range be.GetResults() { + if r.Name == "MIT" { + found = true + } + } + if !found { + t.Errorf("GetResults() did not contain MIT") + } +} + +// TestClassifyLicensesWithContextDone runs a classification with a done +// context, then a normal one on the same backend. The done run must not start +// any work or leave any running, so only the second run's single result shows +// up. This used to fail because the done run kept classifying in the +// background and its results leaked into the second run. +func TestClassifyLicensesWithContextDone(t *testing.T) { + be, err := New() + if err != nil { + t.Fatal(err) + } + path := writeLicense(t) + paths := make([]string, 50) + for i := range paths { + paths[i] = path + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + errs := be.ClassifyLicensesWithContext(ctx, 4, paths, false) + if len(errs) != 1 || errs[0] != context.Canceled { + t.Fatalf("ClassifyLicensesWithContext() = %v, want [%v]", errs, context.Canceled) + } + + if errs := be.ClassifyLicenses(4, []string{path}, false); errs != nil { + t.Fatalf("ClassifyLicenses() = %v", errs) + } + if got := be.GetResults(); len(got) != 1 { + t.Errorf("GetResults() has %d results, want 1", len(got)) + } +}