issues with cleanup on exit

[fortytw2]

I think these lines: https://github.com/chelnak/ysmrr/blob/main/manager.go#L81-L92

Cause problems when the program using ysmrr handles its own exit - I've seen this a bunch on a codebase where the spinner control sequences don't get cleaned up properly, any subsequent uses of the spinner in the same terminal (from different processes) gets pretty borked.

I think factoring this out into something opt-in / ysmrr.HandleInterrupt(sm) / ysmrr.NewSpinnerManager(ysmrr.WithoutInterruptHandler) (any of these could work) would help fix this up.

[chelnak]

I've always been uncomfortable about this. I wonder if it should be there at all?

[fortytw2]

I am unsure of the intricacies of how ClearLine can go wrong if it's not called at the end - looking at what yacspin does, it only seems to have cleanup in terms of concurrency management: https://github.com/theckman/yacspin/blob/master/spinner.go#LL617

[chelnak]

I will try to get some time later and test out a few things.

[chelnak]

Hey @fortytw2 , sorry it's taken so long to get back to you on this.

I'm actually having a hard time replicating the issue.. could be terminal differences or maybe i'm just not understanding the issue properly.

Would you be able to provide a repro? Something that consistently breaks for you?

Thanks

[chelnak]

Thinking about it some more.. The only reason we capture os.Interupt is so that we can ensure that the spinner properly renders on the screen in the "final state".

I wonder if this is even useful..

[chelnak]

With the interupt handler removed it looks like this on a ctrl+c

vs this, with the handler code enabled

The issue i'm finding with the handler code at the moment is that it takes away control from the consuming app.. this doesn't feel right at all.

[chelnak]

argh.. but then the terminal is borked again

[fortytw2]

No worries at all, nothing is urgent here by any means.

Have you reproduced the persistent terminal borkiness thing? Where any subsequent runs of an application using ysmrr print every frame-of-spinners on a new line instead of clearing?

[chelnak]

I haven't managed to replicate it yet..

The description you gave about the bug seems familiar to this: https://github.com/chelnak/ysmrr/pull/34

[VargasRaymondJ]

This little library was perfect for my usecase until I came across what I believe to be a related bug. I'm capturing the os.Interrupt signal in my own application in order to perform some cleanup operations before fulling shutting down. However, due to the os.Exit() my application was terminating prematurely.

[chelnak]

@VargasRaymondJ Yeah this is a killer issue atm. I've not had time to properly debug yet.. though it's been on my mind for a while now.

I'd love to spend some time on this issue soon, but in the meantime if you have suggestions for a fix I'll gladly work with you to get it merged in.

[fortytw2]

I think the internal usage of os.Exit( has to go, pushing signal/exit/cleanup handling responsibilities onto the caller with a breaking api change / v2 release.

I am not familiar enough with how tty control like this works to recommend a way to ensure that every operation is discrete enough that the program can exit at anytime (without cleaning up) without leaving escape codes hanging around that do weird things to future shells/terminals.

[chelnak]

@fortytw2 @VargasRaymondJ

I've removed the code that was trying to handle the interrupts in this pr.

This means that it's the responsibility of the caller to handle any interrupt signals (if desired).

I also updated the ysmrr dependency in gh-changelog and seemed to have no impact on functionality.

The trade-off is that if you ctrl+c the app you get ^Csignal: interrupt .... but I guess that is an expected result.

[chelnak]

👋 I've merged the PR mentioned in the previous comment.

Having played around with different options, the change included seemed to work the best.

I'll continue to test before I cut a new release.

[chelnak]

I've done some more testing and the issues we were seeing here seem to be resolved.. but I appreciate that it's very much a "it works on my computer" thing so let me know you experience any more issues.