Fix CLI output hang after exceptions raised in spinners #2906

kke · 2017-10-04T13:25:07Z

Spinners may leave behind their spinner-threads making Kontena::Cli::Common#puts eat all the messages.

Normally this does not matter because usually the command exits and nobody notices. But when running through Kontena.run! or the way KOSH does it, the threads are left behind and prevent any further putsing.

As a sidenote, the selfmade spinner implementation is far from excellent and should probably be replaced with something like tty-spinner

SpComb · 2017-10-09T13:25:10Z

Do we have an issue for kontena shell freezes without any output if a command fails? How to best to repro this for testing?

SpComb · 2017-10-09T13:31:03Z

Looks like kontena/kontena-plugin-shell#31, fixed in kontena/kontena-plugin-shell#33 for each separate kontena ... command in the shell, but also broken for internal Kontena.run! commands that error out?

SpComb · 2017-10-09T13:36:34Z

cli/lib/kontena/cli/spinner.rb

@@ -168,6 +168,7 @@ def self.spin(msg, &block)
              Kernel.puts msg
            end
          end
+          Thread.main['spinners'].delete(spin_thread)


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?

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.

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

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

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

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

SpComb

I suppose I'll take working puts in rescue from spinner do ... exceptions over "working" spinner nesting.

SpComb · 2017-10-11T13:34:19Z

cli/lib/kontena/cli/spinner.rb

@@ -168,6 +168,7 @@ def self.spin(msg, &block)
              Kernel.puts msg
            end
          end
+          Thread.main['spinners'].delete(spin_thread)


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

SpComb · 2017-10-11T13:48:29Z

cli/lib/kontena/cli/spinner.rb

@@ -168,6 +168,7 @@ def self.spin(msg, &block)
              Kernel.puts msg
            end
          end
+          Thread.main['spinners'].delete(spin_thread)


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

Remove the spinner thread after spinning has finished

0990224

kke added bug cli labels Oct 4, 2017

SpComb reviewed Oct 9, 2017

View reviewed changes

SpComb added this to the 1.4.0 milestone Oct 11, 2017

SpComb approved these changes Oct 11, 2017

View reviewed changes

SpComb changed the title ~~Remove the spinner thread after spinning has finished~~ Fix CLI output hang after exceptions raised in spinners Oct 12, 2017

SpComb merged commit f0c8ed9 into master Oct 12, 2017

SpComb deleted the fix/clear_spinner_threads_on_crash branch October 12, 2017 10:58

SpComb mentioned this pull request Oct 12, 2017

Release v1.4.0.rc2 #2934

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CLI output hang after exceptions raised in spinners #2906

Fix CLI output hang after exceptions raised in spinners #2906

kke commented Oct 4, 2017 •

edited

SpComb commented Oct 9, 2017

SpComb commented Oct 9, 2017

SpComb Oct 9, 2017

SpComb Oct 9, 2017 •

edited

kke Oct 11, 2017

SpComb Oct 11, 2017

kke Oct 11, 2017

SpComb Oct 11, 2017 •

edited

SpComb Oct 11, 2017

SpComb left a comment

SpComb Oct 11, 2017 •

edited

SpComb Oct 11, 2017

Fix CLI output hang after exceptions raised in spinners #2906

Fix CLI output hang after exceptions raised in spinners #2906

Conversation

kke commented Oct 4, 2017 • edited

SpComb commented Oct 9, 2017

SpComb commented Oct 9, 2017

SpComb Oct 9, 2017

Choose a reason for hiding this comment

SpComb Oct 9, 2017 • edited

Choose a reason for hiding this comment

kke Oct 11, 2017

Choose a reason for hiding this comment

SpComb Oct 11, 2017

Choose a reason for hiding this comment

kke Oct 11, 2017

Choose a reason for hiding this comment

SpComb Oct 11, 2017 • edited

Choose a reason for hiding this comment

SpComb Oct 11, 2017

Choose a reason for hiding this comment

SpComb left a comment

Choose a reason for hiding this comment

SpComb Oct 11, 2017 • edited

Choose a reason for hiding this comment

SpComb Oct 11, 2017

Choose a reason for hiding this comment

kke commented Oct 4, 2017 •

edited

SpComb Oct 9, 2017 •

edited

SpComb Oct 11, 2017 •

edited

SpComb Oct 11, 2017 •

edited