Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: terminal clobbering #142

Merged
merged 1 commit into from
Sep 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 33 additions & 3 deletions cmd/quill/internal/ui/ui.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"os"
"strings"
"sync"
"time"

tea "github.com/charmbracelet/bubbletea"
"github.com/charmbracelet/lipgloss"
Expand Down Expand Up @@ -86,11 +87,26 @@ func (m *UI) Teardown(force bool) error {
if !force {
m.handler.State().Running.Wait()
m.program.Quit()
// typically in all cases we would want to wait for the UI to finish. However there are still error cases
// that are not accounted for, resulting in hangs. For now, we'll just wait for the UI to finish in the
// happy path only. There will always be an indication of the problem to the user via reporting the error
// string from the worker (outside of the UI after teardown).
m.running.Wait()
} else {
m.program.Kill()
}
_ = runWithTimeout(250*time.Millisecond, func() error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure I understand the change, we're basically saying, "if we're exiting in a controlled manner, wait for the UI to finish. Otherwise, wait 250 milliseconds for the UI to finish," so that the UI has a better chance to restore the terminal to its normal state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. If we're in a "force shutdown" mode, wait only a short time for the handler and UI to complete. This short time, however, is ages longer than the near instantaneous end of the main thread in the program, which was the culprit for leaving the terminal in a bad state.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation!

m.handler.State().Running.Wait()
return nil
})

m.running.Wait()
// it may be tempting to use Kill() however it has been found that this can cause the terminal to be left in
// a bad state (where Ctrl+C and other control characters no longer works for future processes in that terminal).
m.program.Quit()

_ = runWithTimeout(250*time.Millisecond, func() error {
m.running.Wait()
return nil
})
}

// TODO: allow for writing out the full log output to the screen (only a partial log is shown currently)
// this needs coordination to know what the last frame event is to change the state accordingly (which isn't possible now)
Expand Down Expand Up @@ -200,3 +216,17 @@ func postUIEvents(quiet bool, events ...partybus.Event) {
}
}
}

func runWithTimeout(timeout time.Duration, fn func() error) (err error) {
c := make(chan struct{}, 1)
go func() {
err = fn()
c <- struct{}{}
}()
select {
case <-c:
case <-time.After(timeout):
return fmt.Errorf("timed out after %v", timeout)
}
return err
}
2 changes: 1 addition & 1 deletion internal/log/log.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
var log = discard.New()

func Set(l logger.Logger) {
// though quill the application will automatically have a redaction logger, library consumers may not be doing this.
// though the application will automatically have a redaction logger, library consumers may not be doing this.
// for this reason we additionally ensure there is a redaction logger configured for any logger passed. The
// source of truth for redaction values is still in the internal redact package. If the passed logger is already
// redacted, then this is a no-op.
Expand Down