Opened 9 years ago

Closed 9 years ago

#277 closed defect (fixed)

cmdctl needs nicer warning if address already in use

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

Description

Unfriendly output if attempt to run cmdctl and it is already running (listening on same port):

Traceback (most recent call last):
  File "/udir/jreed/sunstudio-test/src/bin/cmdctl/b10-cmdctl", line 595, in <module>
    run(options.addr, options.port, options.idle_timeout, options.verbose)
  File "/udir/jreed/sunstudio-test/src/bin/cmdctl/b10-cmdctl", line 553, in run
    CommandControl, idle_timeout, verbose)
  File "/udir/jreed/sunstudio-test/src/bin/cmdctl/b10-cmdctl", line 444, in __init__
    http.server.HTTPServer.__init__(self, server_address, RequestHandlerClass)
  File "/udir/jreed/pkg/lib/python3.1/socketserver.py", line 400, in __init__
    self.server_bind()
  File "/udir/jreed/pkg/lib/python3.1/http/server.py", line 127, in server_bind
    socketserver.TCPServer.server_bind(self)
  File "/udir/jreed/pkg/lib/python3.1/socketserver.py", line 411, in server_bind
    self.socket.bind(self.server_address)
socket.error: [Errno 125] Address already in use

(Note if using bind10, this will loop forever.)

Subtickets

Attachments (1)

trac277_cmdctl.diff (3.2 KB) - added by zhanglikun 9 years ago.
The patch for improving the error message when failing to start cmdctl

Download all attachments as: .zip

Change History (10)

comment:1 follow-up: Changed 9 years ago by zhanglikun

  • Owner set to jreed
  • Status changed from new to reviewing

Jeremy, Please have a test for this patch .

comment:2 in reply to: ↑ 1 Changed 9 years ago by jinmei

  • billable set to 1
  • Internal? unset
  • Owner changed from jreed to zhanglikun

Replying to zhanglikun:

Jeremy, Please have a test for this patch .

I'd catch the exception in SecureHTTPServer.init and handle accordingly.

socket.error is pretty broad, and since main is far from the error
happened I'm afraid the error handling could be vague (for the user to
identify the cause of the trouble).

Also, please add a test case for this.

Changed 9 years ago by zhanglikun

The patch for improving the error message when failing to start cmdctl

comment:3 Changed 9 years ago by zhanglikun

  • Owner changed from zhanglikun to jinmei

thanks for you review. Please have a look at the latest patch.

comment:4 Changed 9 years ago by jinmei

  • Owner changed from jinmei to zhanglikun

can you create a short-lived branch so that I can easily run the test?

In this case you should simply be able to do

  • svn cp trunk branch_name
  • svn switch branch_name/../cmdctl (on bin/cmdctl)
  • svn commit
  • svn switch trunk/../cmdctl

please also provide proposed changelog text (if you plan to add it)

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

  • Owner changed from zhanglikun to jinmei

please check r2516 in branch trac277.

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

  • Owner changed from jinmei to zhanglikun

Replying to zhanglikun:

please check r2516 in branch trac277.

  • I'd use CmdctlException? instead of exit. At the very least when we encounter an error of this kind, the exit code should be non 0.
  • as a related note, shouldn't we catch CmdctlException? in main? also, other cases of raising CmdctlException? should be tested (which can go to a different ticket though).
  • I'm not sure if this is tested correctly. doesn't the overridden MySecureHTTPServer.server_bind hide the double-bind situation?
  • 8080 might conflict with other existing processes. in particular, the test fails when BIND 10 is currently running on that machine. So I'd use a different port chosen from a safer range.
  • I'd be more specific in the error message than "Error creating b10-cmdctl". in this case it should be clear that the error is a failure of creating a server object.

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

Replying to jinmei:

  • I'd use CmdctlException? instead of exit. At the very least when we encounter an error of this kind, the exit code should be non 0.

Yes, the CmdctlException? is used now, please check r2521.

  • as a related note, shouldn't we catch CmdctlException? in main? also, other cases of raising CmdctlException? should be tested (which can go to a different ticket though).

Record it in TODO file.

  • I'm not sure if this is tested correctly. doesn't the overridden MySecureHTTPServer.server_bind hide the double-bind situation?

yes, server_bind is overriden, all the communications between fake server and client is processed over temp file.

  • 8080 might conflict with other existing processes. in particular, the test fails when BIND 10 is currently running on that machine. So I'd use a different port chosen from a safer range.

If the specified port is used already, by cmdctl or some other process, the test case "test_addr_in_use" will be skipped. accept your suggestion, selecting one safer port for test.

  • I'd be more specific in the error message than "Error creating b10-cmdctl". in this case it should be clear that the error is a failure of creating a server object.

agree. the error message is changed to 'Error creating server'

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

I've made some proposed changes in the branch (r2522).

If that's okay please go merge the branch to trunk.

comment:9 Changed 9 years ago by zhanglikun

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

Code has been committed to trunk in r2540, so close this ticket.

Note: See TracTickets for help on using tickets.