fix(auth): use a local ServeMux and shut the callback server down cleanly#52
Open
SAY-5 wants to merge 1 commit into
Open
fix(auth): use a local ServeMux and shut the callback server down cleanly#52SAY-5 wants to merge 1 commit into
SAY-5 wants to merge 1 commit into
Conversation
…anly InitiateBrowserAuth registered the /auth/callback handler on http.DefaultServeMux via http.HandleFunc. Any second call in the same process (re-login, tests, library use) panicked with 'http: multiple registrations for /auth/callback'. The server was also never proactively shut down, leaking a goroutine + port binding until the 5-minute timeout fired, and log.Fatalf was called from both goroutines on conditions that aren't fatal (server.ErrServerClosed is the normal Shutdown return). - Build a per-call http.ServeMux and register the handler on it so repeat calls never collide with each other or with other handlers anyone else has on DefaultServeMux. - Replace log.Fatalf with log.Printf on non-fatal conditions and treat http.ErrServerClosed as the expected normal shutdown signal. - Defer server.Shutdown with a 2s timeout so the listener is reaped as soon as InitiateBrowserAuth returns, regardless of which select branch fires. - Give the 5-minute timeout its own select branch so it returns a clear 'authentication timed out' error instead of leaking. Refs flagship-io/abtasty-cli issue 51. Signed-off-by: SAY-5 <say.apm35@gmail.com>
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
1 participant
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.
Fixes #51.
Problem
InitiateBrowserAuth(internal/utils/http_request/common/token.go) had three issues:/auth/callbackonhttp.DefaultServeMuxviahttp.HandleFunc, so any second invocation in the same process (re-login, tests, library use) panicked withhttp: multiple registrations for /auth/callback.log.Fatalfwas called from goroutines on non-fatal conditions (http.ErrServerClosedis the normal Shutdown return), which could kill the whole process unexpectedly.Fix
http.ServeMuxand register the handler on it.server.Shutdownwith a 2-second timeout so the listener is reaped on return, regardless of which select branch fires.http.ErrServerClosedas the expected normal shutdown signal; uselog.Printffor the actual error path.selectbranch and returns a clear "authentication timed out" error.Test
go build ./internal/utils/http_request/common/...clean. (Repo-wide build has pre-existing errors incmd/feature-experimentation/analyze/code-samples/vwo/go/sample-VWO.gothat are unrelated to this change.)