fix(gtfs): honor context cancellation and fix swallowed errors in Adv…#813
Open
tejasva-vardhan wants to merge 1 commit intoOneBusAway:mainfrom
Open
fix(gtfs): honor context cancellation and fix swallowed errors in Adv…#813tejasva-vardhan wants to merge 1 commit intoOneBusAway:mainfrom
tejasva-vardhan wants to merge 1 commit intoOneBusAway:mainfrom
Conversation
…ancedDirectionCalculator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #812
The Problem
While auditing context propagation across the
GtfsManager, I found a resource leak and a swallowed error bug inAdvancedDirectionCalculator.CalculateStopDirection:context.WithoutCancel(ctx)before passing it tocomputeFromShapes. BecausecomputeFromShapesexecutes multiple SQLite queries (GetStopsWithShapeContext,GetShapePointsWithDistance), closing the HTTP connection or hitting a timeout would not cancel the underlying DB work. Under load, these abandoned requests would continue to burn CPU and hold database connections.singleflight.Group.Docallback was implicitly returning anilerror even ifcomputeFromShapesfailed. This meant errors (including standard DB timeouts) were discarded, and thesingleflightwaiters would receive empty strings instead of the actual error.The Solution
context.WithoutCancel(ctx)to ensure SQLite queries correctly honor client cancellation and server shutdowns.singleflight.Group.Docallback to return("", err)when the underlying computation fails, allowing the caller to see the actual error.requestGroup.Doto return an empty string gracefully on failure, matching the existing outward behavior but preventing cache poisoning on transient errors.Behavioral Comparison
computeFromShapesquery keeps running in the background indefinitely. If the database times out or fails, thesingleflightgroup swallows the error and silently caches/returns an empty string"".ctxcancellation propagates down and instantly kills the SQLite query, freeing the DB lock. If an error occurs, it correctly bubbles up from thesingleflightgroup so it can be handled properly without poisoning the cache.Architectural Note / Tradeoff
By honoring
ctxcancellation insidesingleflight, there is a known tradeoff: if the "winning" request that initiated the computation is canceled, other concurrent requests waiting on that samestopIDin the singleflight group will also receive thecontext.Cancelederror. Given that this is an HTTP server, favoring resource reclamation (canceling the DB queries) over guaranteeing the background work finishes is the preferred Go idiom here.