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.

Sign up for GitHub

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

Jump to bottom

Fix CLI output hang after exceptions raised in spinners #2906

Merged
merged 1 commit into from Oct 12, 2017
Merged

Fix CLI output hang after exceptions raised in spinners #2906

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions cli/lib/kontena/cli/spinner.rb
View file
Open in desktop
Expand Up @@ -168,6 +168,7 @@ def self.spin(msg, &block)
Kernel.puts msg
end
end
Thread.main['spinners'].delete(spin_thread)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we delete the spin_thread from Thread.main['spinners'] here in the ensure, then what does the Thread.main['spinners'].pop later on after the ensure block do... because if nothing gets raised, then this will run both the Thread.main['spinners'].delete(spin_thread) and Thread.main['spinners'].pop?

Copy link
Contributor

@SpComb SpComb Oct 9, 2017 •
edited

Choose a reason for hiding this comment

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

Yeah, it does both... the pop is a no-op if the Thread.main['spinners'] is empty:

 [done] Triggering deployment of stack stop-signal     
delete #<Thread:0x000000021da7c0@/home/kontena/kontena/kontena/cli/lib/kontena/cli/spinner.rb:96 dead> from Thread.main['spinners']: [#<Thread:0x000000021da7c0@/home/kontena/kontena/kontena/cli/lib/kontena/cli/spinner.rb:96 dead>]
pop from Thread.main['spinners']: []

I'm guessing this would only be relevant for nested spinners, though... where in the CLI are those actually used? I couldn't find any with a quick look around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hopefully not anywhere. There was one in the master deploy wizard but I think it has been removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could this code be simplified to not be broken in complex cases that are not actually used? :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The whole spinner.rb should be buried. Very deep.

Copy link
Contributor

@SpComb SpComb Oct 11, 2017 •
edited

Choose a reason for hiding this comment

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

The whole spinner.rb should be buried. Very deep.

Nuke it from orbit, it's the only way to be sure

Copy link
Contributor

Choose a reason for hiding this comment

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

Dunno how the nested spinners are supposed to work, but testing by changing the kontena stack install -> Installing dependency ... into a spinner, it actually behaves alright with this buggy change, at least without errors:

 [....] Installing dependency /home/kontena/kontena/kontena/cli/test/deptest-child.yaml as deptest-foo .. \
 [done] Creating stack deptest-foo      
 [done] Triggering deployment of stack deptest-foo     
 [done] Waiting for deployment to start     
 [done] Deploying service redis     
 [done] Installing dependency /home/kontena/kontena/kontena/cli/test/deptest-child.yaml as deptest-foo     
 [....] Installing dependency /home/kontena/kontena/kontena/cli/test/deptest-child.yaml as deptest-bar .. \
 [done] Creating stack deptest-bar      
 [done] Triggering deployment of stack deptest-bar     
 [done] Waiting for deployment to start     
 [done] Deploying service redis     
 [done] Installing dependency /home/kontena/kontena/kontena/cli/test/deptest-child.yaml as deptest-bar     
 [done] Creating stack deptest      
 [done] Triggering deployment of stack deptest     
 [done] Waiting for deployment to start     
 [done] Deploying service redis

In fact, I maybe like the "broken" output with the delete+pop in this PR more than the "working" output before this PR?

 [....] Installing dependency /home/kontena/kontena/kontena/cli/test/deptest-child.yaml as deptest-foo .. \
 [done] Creating stack deptest-foo      
 [....] Installing dependency /home/kontena/kontena/kontena/cli/test/deptest-child.yaml as deptest-foo
 [done] Triggering deployment of stack deptest-foo     
 [....] Installing dependency /home/kontena/kontena/kontena/cli/test/deptest-child.yaml as deptest-foo
 [done] Waiting for deployment to start     
 [....] Installing dependency /home/kontena/kontena/kontena/cli/test/deptest-child.yaml as deptest-foo .. |
 [done] Deploying service redis     
 [done] Installing dependency /home/kontena/kontena/kontena/cli/test/deptest-child.yaml as deptest-foo     
 [....] Installing dependency /home/kontena/kontena/kontena/cli/test/deptest-child.yaml as deptest-bar .. \
 [done] Creating stack deptest-bar      
 [....] Installing dependency /home/kontena/kontena/kontena/cli/test/deptest-child.yaml as deptest-bar
 [done] Triggering deployment of stack deptest-bar     
 [....] Installing dependency /home/kontena/kontena/kontena/cli/test/deptest-child.yaml as deptest-bar
 [done] Waiting for deployment to start     
 [....] Installing dependency /home/kontena/kontena/kontena/cli/test/deptest-child.yaml as deptest-bar .. \
 [done] Deploying service redis     
 [done] Installing dependency /home/kontena/kontena/kontena/cli/test/deptest-child.yaml as deptest-bar     
 [done] Creating stack deptest      
 [done] Triggering deployment of stack deptest     
 [done] Waiting for deployment to start     
 [done] Deploying service redis

end

exit(status) if status
Expand Down