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

Handle when parts is empty in MultiPrinter #640

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AlexThurston
Copy link

It seems as though in certain bases parts can be blank.

Copy link

codecov bot commented Feb 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1a9d517) 81.05% compared to head (36a0678) 81.01%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #640      +/-   ##
==========================================
- Coverage   81.05%   81.01%   -0.04%     
==========================================
  Files          33       33              
  Lines        4176     4178       +2     
==========================================
  Hits         3385     3385              
- Misses        718      720       +2     
  Partials       73       73              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cwinters8
Copy link

I think I'm running into the problem this PR attempts to solve. When I run something like this using pterm v0.12.79:

pterm.DefaultBasicText.WithWriter(multi.NewWriter()).Println()

I get a panic with this error:

panic: runtime error: index out of range [-1]

goroutine 8 [running]:
github.com/pterm/pterm.(*MultiPrinter).getString(0xc00006c000?)
	/Users/clark/go/pkg/mod/github.com/pterm/pterm@v0.12.79/multi_live_printer.go:67 +0x1c6
github.com/pterm/pterm.(*MultiPrinter).Start.func1()
	/Users/clark/go/pkg/mod/github.com/pterm/pterm@v0.12.79/multi_live_printer.go:88 +0x25
atomicgo.dev/schedule.Every.func1()
	/Users/clark/go/pkg/mod/atomicgo.dev/schedule@v0.1.0/schedule.go:106 +0x8f
created by atomicgo.dev/schedule.Every in goroutine 1
	/Users/clark/go/pkg/mod/atomicgo.dev/schedule@v0.1.0/schedule.go:102 +0x14c

I added a dependency replacement in go.mod to try to use these changes

replace github.com/pterm/pterm v0.12.79 => github.com/AlexThurston/pterm v0.0.0-20240211181148-36a06783aa1b

which eliminates the panic, but nothing ends up getting printed, and the program hangs. When I comment out the empty Println(), the program behaves normally.

Full reproduction of the issue on Replit here

I believe it hangs because that for loop will never end, because s will always be empty

for s == "" {
  if len(parts) != 0 {
    parts = parts[:len(parts)-1]
    if len(parts) != 0 {
      s = parts[len(parts)-1]
    }
  }
}

I think the loop's condition needs to check if len(parts) > 0. I updated the loop in multi_live_printer.go locally, and it fixed both the panic and the hang:

for s == "" && len(parts) > 0 {
  parts = parts[:len(parts)-1]
  if len(parts) != 0 {
    s = parts[len(parts)-1]
  }
}

@AlexThurston would you be willing to update to that (or similar) logic and see if the tests still function as expected?

@starcorn2020
Copy link

starcorn2020 commented Mar 31, 2024

I encountered the same issue, experiencing a Panic under unexpected conditions. Here is the error message. I tried using the solution provided by the author, but print message will format wrong;

Goroutine 77 [running]:
github.com/pterm/pterm.(*MultiPrinter).getString(0xc00007c600?)
/home/ec2-user/go/pkg/mod/github.com/pterm/pterm@v0.12.79/multi_live_printer.go:67 +0x1c6
github.com/pterm/pterm.(*MultiPrinter).Start.func1()
/home/ec2-user/go/pkg/mod/github.com/pterm/pterm@v0.12.79/multi_live_printer.go:88 +0x25
atomicgo.dev/schedule.Every.func1()
/home/ec2-user/go/pkg/mod/atomicgo.dev/schedule@v0.1.0/schedule.go:106 +0x8f
created by atomicgo.dev/schedule.Every in goroutine 8
/home/ec2-user/go/pkg/mod/atomicgo.dev/schedule@v0.1.0/schedule.go:102 +0x15b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants