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 pager not being closed after command failure #729

Open
bmeneg opened this issue Aug 10, 2021 · 0 comments
Open

Fix pager not being closed after command failure #729

bmeneg opened this issue Aug 10, 2021 · 0 comments

Comments

@bmeneg
Copy link
Collaborator

bmeneg commented Aug 10, 2021

The way the pager is getting closed today is by using deferred code:

lab/cmd/ci_status.go

Lines 52 to 53 in 5a1fd5d

pager := newPager(cmd.Flags())
defer pager.Close()

However, in case an error occurs in the middle of a pager session and a log.Fatal() is used the pager.Close() is never called.
Per Golang documentation, the log.Fatal() call terminates the program abruptly, without cleaning or running any deferred code.

In this specific case, when the pager isn't closed properly, the stdout is kept allocated to lab in that console session, meaning that the user won't be able to write anything to the console line.

I noticed this behavior during tests with lab ci commands, where I did a typo and the command failed to find what I was requesting and exit'ed the program:

$ lab ci status --merge-request --bridge 'realtime_check_regularu' upstream 1128
2021/08/10 19:58:42 ERROR: ci_status.go:88: no CI jobs found in pipeline 351155952 on remote redhat/rhel/src/kernel/rhel-8
$ ^C                <--- I was trying to write something, but only the control character was shown
$ ^C

I didn't find any way to force deferred code to run before the log.Fatal() is actually called (within our custom log lib).
With that, we have 2 ways of correcting it:

  1. stop using log.Fatal() as a whole, but return errors until the point where there are no more code in the stack to move back.
  2. change our custom log lib to instead of using the builtin log.Fatal() function to use Panic() plus Rescue() instead, creating an exception handling mechanism.

The option 2) is quite tempting to try and I would be happy to do so, however, it'll take countless more time to get done right than the option 1). With that, I suggest moving towards 1) and let 2) for the future.

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

No branches or pull requests

1 participant