Add synchronized spinner API with direct tea.Program integration #4351
Conversation
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
Implements a new spinner API (NewSpinner/Update/Close) that directly interacts with tea.Program without intermediate goroutines. The new interface provides proper synchronization via Close() which waits for the spinner to fully terminate. Key changes: - spinner struct now holds tea.Program directly and sends messages via p.Send() - Update() sends messages directly to tea.Program (no channel bridge) - Close() properly synchronizes and waits for tea.Program to finish - Old Spinner() API now wraps NewSpinner with a goroutine bridge for backward compatibility Benefits: - New API: Cleaner, direct message passing, proper synchronization - Old API: 100% backward compatible, all 193 existing usages work unchanged - Reduced goroutines: 2 per new spinner vs 3 in old implementation Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Converts all manual calls from cmdio.Spinner() to cmdio.NewSpinner() across the codebase, excluding auto-generated cmd/workspace and cmd/account directories. Changes: - bundle/run: Use sp.Update() and explicit sp.Close() after operations - cmd/labs: Use sp.Update() with defer sp.Close() for multi-step ops - cmd/psql, cmd/selftest: Use new API with explicit Close() timing - experimental/ssh: Use new API with explicit Close() after loading - libs/databrickscfg, libs/template: Use new API matching original timing The refactoring preserves exact Close() timing: - defer sp.Close() where original had "defer close()" - explicit sp.Close() where original had immediate "close()" All tests pass and linter is clean. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
pietern
requested review from
alexott,
andrewnester,
anton-107,
denik,
lennartkats-db,
nfx and
shreyas-goenka
as code owners
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
|
Commit: 475b0fb
16 interesting tests: 7 KNOWN, 5 SKIP, 4 RECOVERED
Top 50 slowest tests (at least 2 minutes):
|
The previous implementation had a race condition where concurrent calls to Close() (e.g., from context cancellation and explicit user call) would not all wait for the tea.Program to terminate. Problem: - First caller: Executes once.Do body, sends quitMsg, waits on <-done - Second caller: once.Do skips body, returns immediately WITHOUT waiting This meant the second caller could return before the spinner fully terminated, violating the Close() contract. Fix: - Move the wait outside once.Do so ALL callers wait for termination - Only the first caller sends quitMsg (via once.Do) - All callers wait for <-done (outside once.Do) Added TestSpinnerStructConcurrentClose to verify concurrent Close() calls from multiple goroutines work correctly without races. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
pietern
force-pushed
the
cmdio-spinner-new-api
branch
from
4ac09ae to
475b0fb
Compare
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
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.
Problem
The previous
cmdio.Spinner()API returned immediately onclose(spinner)without waiting for the spinner to terminate. Callers had no way to ensure the spinner fully stopped before proceeding. This potentially races with log statements or program termination (failing to restore cursor state). As of #4336 we perform synchronization on program termination, but this addresses the symptom not the cause.Solution
New
NewSpinner()API with synchronousClose()that blocks until tea.Program terminates:Changes:
sp.Update()sends directly to tea.Program (no bridge goroutines)sp.Close()blocks until cleanup completes (guaranteed termination)Close()calls (context cancellation + explicit)Refactored 9 manual usages. All tests pass with race detector.