fix(places): apply Timeout when HTTPClient has none - #28
Conversation
NewClient only set Timeout when HTTPClient was nil. Callers that
passed &http.Client{} (or any client with Timeout 0) ignored
Options.Timeout and hung forever on a stalled Places request.
Clone the provided client and apply Timeout (default 10s) when it
is still zero. Explicit non-zero timeouts are left alone.
Red: go test ./internal/places -run TestNewClientAppliesTimeoutToProvidedClient
timeout=0s want 2s
Green: same command ok
Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Codex review: found issues before merge. Reviewed September 1, 2026, 5:06 PM ET / 21:06 UTC. ClawSweeper reviewWhat this changesThis branch clones a caller-provided Go HTTP client with no timeout and applies the configured or default Places-client timeout while preserving explicit client timeouts. Regression provenancePossible regression — probable (reviewed change; reproduction). No predecessor PR is attributed. Merge readiness⛔ Blocked by patch quality or review findings - 6 items remain Keep open for an explicit maintainer compatibility decision: current main leaves a supplied zero-timeout HTTP client unchanged, while this branch silently imposes the configured or default 10-second deadline on existing library callers. Priority: P1 Review scores
Verification
How this fits togetherThe public Go library turns caller options into a Places client, which all search and lookup methods use for outbound HTTP requests. CLI flags provide a timeout to the default library client, while embedding callers may provide their own HTTP client and request context. flowchart LR
A[Go caller or CLI] --> B[Client options]
B --> C[Client construction]
C --> D{Supplied HTTP client?}
D -->|No| E[Default timeout client]
D -->|Yes| F[Caller client policy]
E --> G[Places request transport]
F --> G
G --> H[Google Places API]
Decision needed
Why: Both policies are viable, but selecting either changes the exported library contract and cannot be resolved safely as a patch-level implementation choice. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Preserve supplied-client zero-timeout semantics; if timeout inheritance is desired, introduce it as an explicitly documented and versioned public contract with migration and compatibility scenarios. Do we have a high-confidence way to reproduce the issue? Yes. The collaborator review exercised the public Go library through a real loopback TCP server on current main and the exact PR head, including the longer-than-10-second response that differs between them. Is this the best way to solve the issue? No. The new test proves the proposed policy but not that it is compatible; preserving caller-supplied client control is the narrow safe path unless maintainers deliberately adopt and document a new contract. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning high; reviewed against 77023d8abb4c. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (42 earlier review cycles; latest 8 shown)
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
clawsweeper
Bot
added
status: 📣 needs proof
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Maintainer triage: I reproduced the behavior through the public Go library over real loopback TCP, using current main The patch applies the requested timeout, but it also changes existing supplied-client behavior. With Recommendation: close this PR as currently proposed, subject to Peter's decision on the public contract. Preserve caller-supplied client control. If timeout inheritance is desired, it needs an explicit compatibility decision and documented migration under The CLI claim is also inaccurate:
Every scenario reached the TCP server exactly once. The supplied client's Timeout field remained unchanged. The PR's This proof uses synthetic local responses and a dummy local-only key. It does not call Google or validate Google account/billing behavior. Standalone reproduction source and commandsSave the following as go build -o /tmp/goplaces-probe-main /path/to/probe.go
go build -o /tmp/goplaces-main ./cmd/goplaces
/tmp/goplaces-probe-main main /tmp/goplaces-mainFrom a checkout of PR head go test ./...
go build -o /tmp/goplaces-probe-pr28 /path/to/probe.go
/tmp/goplaces-probe-pr28 pr28package main
import (
"context"
"errors"
"fmt"
"io"
"net"
"net/http"
"os"
"os/exec"
"strings"
"sync/atomic"
"time"
"github.com/steipete/goplaces"
)
func main() {
revision := os.Args[1]
var requests atomic.Int64
listener, err := net.Listen("tcp", "127.0.0.1:0")
if err != nil {
panic(err)
}
server := &http.Server{Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.Method != http.MethodPost || !strings.HasSuffix(r.URL.Path, "/places:searchText") {
http.Error(w, "unexpected request", 400)
return
}
requests.Add(1)
_, _ = io.Copy(io.Discard, r.Body)
delay := 600 * time.Millisecond
if strings.HasPrefix(r.URL.Path, "/long/") {
delay = 10500 * time.Millisecond
}
timer := time.NewTimer(delay)
defer timer.Stop()
select {
case <-r.Context().Done():
return
case <-timer.C:
w.Header().Set("Content-Type", "application/json")
_, _ = io.WriteString(w, `{"places":[{"id":"synthetic-place","displayName":{"text":"Synthetic Cafe"}}]}`)
}
})}
go func() { _ = server.Serve(listener) }()
defer server.Close()
baseURL := "http://" + listener.Addr().String()
scenarios := []struct {
name string
provided *http.Client
option, contextLimit time.Duration
path string
mainDeadline, prDeadline bool
}{
{"custom_zero_explicit_option", &http.Client{}, 100 * time.Millisecond, 350 * time.Millisecond, "/short", true, true},
{"custom_explicit_wins", &http.Client{Timeout: 200 * time.Millisecond}, 100 * time.Millisecond, 350 * time.Millisecond, "/short", true, true},
{"nil_client_option", nil, 100 * time.Millisecond, 350 * time.Millisecond, "/short", true, true},
{"custom_zero_default_long_context", &http.Client{}, 0, 12 * time.Second, "/long", false, true},
}
for _, scenario := range scenarios {
client := goplaces.NewClient(goplaces.Options{APIKey: "synthetic-local-only", BaseURL: baseURL + scenario.path, HTTPClient: scenario.provided, Timeout: scenario.option})
ctx, cancel := context.WithTimeout(context.Background(), scenario.contextLimit)
before := requests.Load()
started := time.Now()
response, callErr := client.Search(ctx, goplaces.SearchRequest{Query: "coffee", Limit: 1})
elapsed := time.Since(started)
deadline := errors.Is(callErr, context.DeadlineExceeded)
contextExpired := ctx.Err() != nil
cancel()
original := "nil"
if scenario.provided != nil {
original = scenario.provided.Timeout.String()
}
fmt.Printf("%s %s elapsed=%s deadline=%v context_expired=%v results=%d original_timeout=%s network_requests=%d\n", revision, scenario.name, elapsed.Round(time.Millisecond), deadline, contextExpired, len(response.Results), original, requests.Load()-before)
expected := scenario.mainDeadline
if revision == "pr28" {
expected = scenario.prDeadline
}
if deadline != expected || requests.Load()-before != 1 || (!deadline && (callErr != nil || len(response.Results) != 1)) {
panic(fmt.Sprintf("unexpected result: %v", callErr))
}
}
if len(os.Args) > 2 {
cmd := exec.Command(os.Args[2], "--timeout=100ms", "--json", "search", "coffee", "--limit=1")
cmd.Env = []string{"GOOGLE_PLACES_API_KEY=synthetic-local-only", "GOOGLE_PLACES_BASE_URL=" + baseURL + "/short", "NO_COLOR=1"}
started := time.Now()
output, cliErr := cmd.CombinedOutput()
code := 0
if cliErr != nil {
var e *exec.ExitError
if !errors.As(cliErr, &e) {
panic(cliErr)
}
code = e.ExitCode()
}
clean := strings.ReplaceAll(string(output), baseURL, "http://127.0.0.1:PORT")
fmt.Printf("%s built_cli elapsed=%s exit=%d stderr=%s", revision, time.Since(started).Round(time.Millisecond), code, clean)
if code == 0 || !strings.Contains(clean, "deadline exceeded") {
panic("CLI did not enforce timeout")
}
}
} |
clawsweeper
Bot
added
proof: sufficient
Closing as requested. A supplied |
2 participants
What Problem This Solves
Fixes an issue where callers that pass their own
http.Client(including an empty&http.Client{}) never getOptions.Timeout. A stalled Google Places TCP connection then hangs the request forever.The CLI already sets
Timeout(default 10s). That value was ignored wheneverHTTPClientwas non-nil.Why This Change Was Made
NewClientnow appliesOptions.Timeout(default 10s) when the provided client hasTimeout == 0. The provided client is cloned so the caller is not mutated. An explicit non-zeroHTTPClient.Timeoutis kept.User Impact
Library and CLI callers that pass a custom client without a timeout now get the same 10s bound as the default client. Callers that already set a timeout are unchanged.
Evidence
Before (unfixed
NewClientwith&http.Client{}andTimeout: 2s):After this patch:
The caller’s original client stays at
Timeout 0. An explicit 7s client timeout is preserved.Real behavior proof
Behavior or issue addressed: Provided HTTP clients with no Timeout ignored Options.Timeout and could hang forever on a stalled Places request.
Real environment tested: macOS, Go toolchain, module checkout at
/tmp/gp-http-timeout.Exact steps or command run after this patch:
Evidence after fix: terminal output from the patched tree:
Observed result after fix: A provided client with Timeout 0 now receives Options.Timeout (2s in the probe, 10s default). Explicit non-zero timeouts stay as the caller set them.
What was not tested: Live Google Places API against a real stalled TCP peer.