fix: propagate context into static GTFS HTTP download#536
fix: propagate context into static GTFS HTTP download#536suhr25 wants to merge 3 commits intoOneBusAway:mainfrom
Conversation
rawGtfsData used http.NewRequest (no context), so ForceUpdate's caller context was never linked to the HTTP request. Switch to http.NewRequestWithContext and thread ctx through loadGTFSData and rawGtfsData so cancellations and deadlines take effect. Signed-off-by: suhr25 <suhridmarwah07@gmail.com>
|
Hi @aaronbrethorst, |
There was a problem hiding this comment.
Hey Suhrid — clean fix. The ForceUpdate method accepted a context but the HTTP request underneath was ignoring it entirely. Threading ctx through loadGTFSData → rawGtfsData → http.NewRequestWithContext is exactly the right fix, and keeping context.Background() for the startup path in InitGTFSManager is correct since there's no meaningful caller context at that point.
Critical
No critical issues.
Important
No important issues.
Fit and Finish
No fit-and-finish issues. Single commit, minimal diff, correct change.
Note
PR #513 is also modifying InitGTFSManager and loadGTFSData to accept context.Context. You'll need to fix this merge conflict before I can merge your PR.
…ry logic - InitGTFSManager now accepts context.Context and propagates it through loadGTFSData and buildGtfsDB instead of using context.Background() - Preserve retry loop from main: configurable backoffs, cancellable sleeps, partial DB cleanup between attempts, and retry success logging - buildGtfsDB signature updated to accept context.Context; ForceUpdate call site updated accordingly - Add StartupRetries []time.Duration field to Config for configurable backoffs - Update all InitGTFSManager call sites to pass context.Background()
Resolve conflicts keeping ctx passed through InitGTFSManager, loadGTFSData, and BuildApplication call chains. Retain StartupRetries field comment and retry/backoff logic from fix branch.
|
Hi @aaronbrethorst, |
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
Hey Suhrid, thanks for resolving the merge conflict. The context propagation fix itself is still clean, and the conflict resolution correctly preserved the ctx threading through the startup retry loop in InitGTFSManager.
One small thing to fix before we can merge:
Fit and Finish
- Run
go fmt— The merge conflict resolution introduced inconsistent struct field alignment ininternal/gtfs/config.go. Lines 28–31 (Env,Verbose,EnableGTFSTidy,StartupRetries) use narrower alignment than the fields above them. Runninggo fmt ./...will fix this automatically.
Thanks again, and I look forward to merging this change.
2 participants
This PR makes GTFS downloads respect context cancellation. Previously,
ForceUpdate(ctx)accepted a context but the HTTP request ignored it. Changes are mainly ininternal/gtfs/static.go, with small updates ingtfs_manager.go.FIX
ForceUpdatehad no effect.ctxthroughloadGTFSData→rawGtfsDataand usehttp.NewRequestWithContext.VERIFICATION
Run:
go test ./...Test with a slow GTFS endpoint and a short timeout:
ForceUpdatewith ~500ms contextcontext deadline exceeded