Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#588 closed defect (fixed)

bindctl traceback on shutdown

Reported by: jreed Owned by: zzchen_pku
Priority: low Milestone: A-Team-Sprint-20110316
Component: ~bind-ctl (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 3.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

I just did "echo Boss shutdown | bindctl" and received:

["login success "] login as root
Error:  
Traceback (most recent call last):
  File "/home/reed/opt/bind10/bin/bindctl", line 133, in <module>
    tool.run()
  File "/home/reed/opt/bind10/lib/python3.1/site-packages/bindctl/bindcmd.py", line 122, in run
    self.cmdloop()
  File "/usr/pkg/lib/python3.1/cmd.py", line 138, in cmdloop
    line = self.precmd(line)
  File "/home/reed/opt/bind10/lib/python3.1/site-packages/bindctl/bindcmd.py", line 271, in precmd
    self._update_all_modules_info()
  File "/home/reed/opt/bind10/lib/python3.1/site-packages/bindctl/bindcmd.py", line 265, in _update_all_modules_info
    self.config_data.update_specs_and_config()
  File "/home/reed/opt/bind10/lib/python3.1/site-packages/isc/config/ccsession.py", line 365, in update_specs_and_config
    self.request_specifications();
  File "/home/reed/opt/bind10/lib/python3.1/site-packages/isc/config/ccsession.py", line 360, in request_specifications
    specs = self._conn.send_GET('/module_spec')
  File "/home/reed/opt/bind10/lib/python3.1/site-packages/bindctl/bindcmd.py", line 232, in send_GET
    status, reply_msg = self._send_message(url, body)
  File "/home/reed/opt/bind10/lib/python3.1/site-packages/bindctl/bindcmd.py", line 224, in _send_message
    self.conn.request('GET', url, body, headers)
  File "/usr/pkg/lib/python3.1/http/client.py", line 918, in request
    self._send_request(method, url, body, headers)
  File "/usr/pkg/lib/python3.1/http/client.py", line 946, in _send_request
    self.putrequest(method, url, **skips)
  File "/usr/pkg/lib/python3.1/http/client.py", line 810, in putrequest
    raise CannotSendRequest(self.__state)
http.client.CannotSendRequest: Request-sent

bind10 did stop.

(By the way, probably non-interactive bindctl use should not say: login success? login as root.)

Subtickets

Change History (16)

comment:1 Changed 9 years ago by jreed

  • Milestone changed from feature backlog item to A-Team-Sprint-20110309
  • Priority changed from major to minor

comment:2 Changed 9 years ago by zzchen_pku

  • Owner set to zzchen_pku
  • Status changed from new to assigned

comment:3 Changed 9 years ago by zzchen_pku

  • Owner changed from zzchen_pku to UnAssigned
  • Status changed from assigned to reviewing

Done, handled the exception and hide related log info while using non-interactive bindctl.

comment:4 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:5 follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to zzchen_pku

Hello

Shouldn't this have some tests? Not that I would have an idea how to test it, but some kind would be nice.

Shouldn't this have a changelog entry?

Anyway, while this hides the stacktrace on EOF (ctrl+d, etc), it also hides it on real errors and pretends everything is OK. For example, if I do:

> Boss shutdown
> Boss shutdown

Exit from bindctl

It may be misleading to think that it just happily terminated. We should know when submitting of the command failed because there was noone to submit to (either by a traceback or some error message).

I also fixed one space, which wasn't by the recommended code style.

Thanks

comment:6 in reply to: ↑ 5 ; follow-up: Changed 9 years ago by zzchen_pku

  • Owner changed from zzchen_pku to vorner

Replying to vorner:

Shouldn't this have some tests? Not that I would have an idea how to test it, but some kind would be nice.

Since the bug was rised by

status, reply_msg = self._send_message(url, body)

I rewrite cmdloop() logic, hope it make sense, or any better suggestions?

Shouldn't this have a changelog entry?

I think it should have one, please find below:

193.  [bug]     jerry
  src/bin/bindctl: bindctl shouldn't show traceback on shutdown.
  (Trac #588, git TDB)

It may be misleading to think that it just happily terminated. We should know when submitting of the command failed because there was noone to submit to (either by a traceback or some error message).

Updated, interactive and non-interactive mode will show different log message.

I also fixed one space, which wasn't by the recommended code style.

Thanks.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to zzchen_pku

Hello

Replying to zzchen_pku:

Shouldn't this have a changelog entry?

I think it should have one, please find below:

193.  [bug]     jerry
  src/bin/bindctl: bindctl shouldn't show traceback on shutdown.
  (Trac #588, git TDB)

That's true, but strictly speaking, it was true even before this change ;-). May I propose „bindctl doesn't show traceback on shutdown“?

It may be misleading to think that it just happily terminated. We should know when submitting of the command failed because there was noone to submit to (either by a traceback or some error message).

Updated, interactive and non-interactive mode will show different log message.

We may have misunderstood each other here little bit.

My concern wasn't about the difference between interactive and noninteractive, but about the difference with „just couldn't say good bye on shutdown“ vs. „couldn't send a command user typed“. So, what I mean is it should do something like this:

If I shutdown and exit, it shoud be OK:

> Boss shutdown
# Now the bind10 shuts down
> ^D #I type ctrl+D to send an EOF ‒ I want it to shutdown
# It should just happily terminate, possibly with ani kind of warm goodbye message like „Exit from bindctl“ or whatever

But if I (or someone else) shuts down and then send a command, it should complain:

> Boss shutdown
# Now it terminates
> Boss shutdown
Error: Help, help, call the ops and cops and everybody and... someone killed our beloved cmdctl!!!

I believe this should be the same regardless of interactivity.

Is it clearer what I mean?

Anyway, you maybe removed something that should stay there? I got another backtrace on exit once (not always, so it might be some kind of race condition):

["login success "] login as root
> Boss shutdown
# Here was ^D
> Traceback (most recent call last):
  File "./bin/bindctl", line 133, in <module>
    tool.run()
  File "/home/vorner/testing/bind10/lib64/python3.1/site-packages/bindctl/bindcmd.py", line 122, in run
    self.cmdloop()
  File "/usr/lib64/python3.1/cmd.py", line 138, in cmdloop
    line = self.precmd(line)
  File "/home/vorner/testing/bind10/lib64/python3.1/site-packages/bindctl/bindcmd.py", line 278, in precmd
    self._update_all_modules_info()
  File "/home/vorner/testing/bind10/lib64/python3.1/site-packages/bindctl/bindcmd.py", line 272, in _update_all_modules_info
    self.config_data.update_specs_and_config()
  File "/home/vorner/testing/bind10/lib64/python3.1/site-packages/isc/config/ccsession.py", line 365, in update_specs_and_config
    self.request_specifications();
  File "/home/vorner/testing/bind10/lib64/python3.1/site-packages/isc/config/ccsession.py", line 360, in request_specifications
    specs = self._conn.send_GET('/module_spec')
  File "/home/vorner/testing/bind10/lib64/python3.1/site-packages/bindctl/bindcmd.py", line 239, in send_GET
    status, reply_msg = self._send_message(url, body)
  File "/home/vorner/testing/bind10/lib64/python3.1/site-packages/bindctl/bindcmd.py", line 231, in _send_message
    self.conn.request('GET', url, body, headers)
  File "/usr/lib64/python3.1/http/client.py", line 949, in request
    self._send_request(method, url, body, headers)
  File "/usr/lib64/python3.1/http/client.py", line 977, in _send_request
    self.putrequest(method, url, **skips)
  File "/usr/lib64/python3.1/http/client.py", line 832, in putrequest
    raise CannotSendRequest(self.__state)
http.client.CannotSendRequest: Request-sent

Thanks

comment:8 in reply to: ↑ 7 Changed 9 years ago by zzchen_pku

  • Owner changed from zzchen_pku to vorner

Replying to vorner:

That's true, but strictly speaking, it was true even before this change ;-). May I propose „bindctl doesn't show traceback on shutdown“?

The change is ok for me.

It may be misleading to think that it just happily terminated. We should know when submitting of the command failed because there was noone to submit to (either by a traceback or some error message).

We may have misunderstood each other here little bit.

Got it, has updated the exception handling logic accordingly.

["login success "] login as root
> Boss shutdown
# Here was ^D
> Traceback (most recent call last):
  File "./bin/bindctl", line 133, in <module>
    tool.run()
  File "/home/vorner/testing/bind10/lib64/python3.1/site-packages/bindctl/bindcmd.py", line 122, in run
    self.cmdloop()
  File "/usr/lib64/python3.1/cmd.py", line 138, in cmdloop
    line = self.precmd(line)
  File "/home/vorner/testing/bind10/lib64/python3.1/site-packages/bindctl/bindcmd.py", line 278, in precmd
    self._update_all_modules_info()
  File "/home/vorner/testing/bind10/lib64/python3.1/site-packages/bindctl/bindcmd.py", line 272, in _update_all_modules_info
    self.config_data.update_specs_and_config()
  File "/home/vorner/testing/bind10/lib64/python3.1/site-packages/isc/config/ccsession.py", line 365, in update_specs_and_config
    self.request_specifications();
  File "/home/vorner/testing/bind10/lib64/python3.1/site-packages/isc/config/ccsession.py", line 360, in request_specifications
    specs = self._conn.send_GET('/module_spec')
  File "/home/vorner/testing/bind10/lib64/python3.1/site-packages/bindctl/bindcmd.py", line 239, in send_GET
    status, reply_msg = self._send_message(url, body)
  File "/home/vorner/testing/bind10/lib64/python3.1/site-packages/bindctl/bindcmd.py", line 231, in _send_message
    self.conn.request('GET', url, body, headers)
  File "/usr/lib64/python3.1/http/client.py", line 949, in request
    self._send_request(method, url, body, headers)
  File "/usr/lib64/python3.1/http/client.py", line 977, in _send_request
    self.putrequest(method, url, **skips)
  File "/usr/lib64/python3.1/http/client.py", line 832, in putrequest
    raise CannotSendRequest(self.__state)
http.client.CannotSendRequest: Request-sent

Added, please check.

comment:9 follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to zzchen_pku

It seems everything is working like it should, in nice and friendly manner. And the test about the failure is more thorough. I just fixed some small typos.

Anyway, would it be possible to test the EOF case as well? I think this part of code isn't tested (but I might have overlooked it, maybe it's tested in some already existing test or something).

Thank you

comment:10 in reply to: ↑ 9 Changed 9 years ago by zzchen_pku

  • Owner changed from zzchen_pku to vorner

Replying to vorner:

Anyway, would it be possible to test the EOF case as well? I think this part of code isn't tested (but I might have overlooked it, maybe it's tested in some already existing test or something).

Added a straightforward test for it, since the EOF case logic is also quite straightforward, is this ok?

comment:11 follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to zzchen_pku

Hello

I noticed that the test was actually broken. You executed the precmd before calling assertRaises, so it's result got passed to it. It then tried to call it instead of the tested function. And None as an exception seems to mean it throws something and I don't care what, so when it tried to call the return value, the check was happy, because it threw that this is not callable.

I didn't find a function that checks it doesn't throw anything, so I left the call just outside anything. It would make the test fail as well if it was broken.

So, if you agree with this change, it can be merged.

Anyway, not directly related to this ticket, I noticed that no our code calls onecmd and precmd. Do you have an idea what calls them? Just out of curiosity?

Thanks

comment:12 in reply to: ↑ 11 Changed 9 years ago by zzchen_pku

Replying to vorner:

So, if you agree with this change, it can be merged.

Okay,I'll merge it.

Anyway, not directly related to this ticket, I noticed that no our code calls onecmd and precmd. Do you have an idea what calls them? Just out of curiosity?

Because BindCmdInterpreter? is a subclass of Cmd, you can find the code calls in cmd.py:

def cmdloop(self, intro=None): 
  ...
  line = self.precmd(line)        
  stop = self.onecmd(line)        
  stop = self.postcmd(stop, line) 
  ...

Hope it is helpful:-)
Thanks for your review.

comment:13 Changed 9 years ago by zzchen_pku

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged,closing it.

comment:14 follow-up: Changed 9 years ago by jinmei

Just happened to notice: we should add a hindsight "estimation" of this task.
Please do > Jerry

comment:15 Changed 9 years ago by zzchen_pku

  • Estimated Difficulty changed from 0.0 to 3.0

comment:16 in reply to: ↑ 14 Changed 9 years ago by zzchen_pku

Replying to jinmei:

Just happened to notice: we should add a hindsight "estimation" of this task.
Please do > Jerry

Done, thanks.

Note: See TracTickets for help on using tickets.