Opened 7 years ago

Closed 7 years ago

#2004 closed defect (fixed)

Message_makeResponse (libdns++ python wrapper) should catch exceptions

Reported by: jinmei Owned by: muks
Priority: medium Milestone: Sprint-20120612
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 0.5 Internal?: no

Description

It propagates some exceptions, making the caller python script crash.
The fix should be easy, so it probably makes sense if we fix this
type of bug in the Message class comprehensively.

Subtickets

Change History (11)

comment:1 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120612

comment:2 follow-up: Changed 7 years ago by muks

  • Owner changed from UnAssigned to muks
  • Status changed from new to assigned

Picking.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 7 years ago by jinmei

Replying to muks:

Picking.

Could you first complete the things you are currently working on? Or, if you get stuck
with the existing ones with difficulty, could you release it and ask someone else for
taking over it?

comment:4 in reply to: ↑ 3 ; follow-up: Changed 7 years ago by muks

Replying to jinmei:

Replying to muks:

Picking.

Could you first complete the things you are currently working on? Or, if you get stuck
with the existing ones with difficulty, could you release it and ask someone else for
taking over it?

I am working on my tickets (as you can see from commit logs and ticket updates). One ticket takes time to do a full check each time, and for another I am reading before I can fix it. So it's best if I put another ticket that I _can_ fix to review before I do that so that in a week, i would have fixed some things.

I also don't have more than 1 ticket in 'assigned' at any time (except once when I had already finished a ticket and was waiting for check to finish).

comment:5 in reply to: ↑ 4 Changed 7 years ago by jinmei

Replying to muks:

Could you first complete the things you are currently working on? Or, if you get stuck
with the existing ones with difficulty, could you release it and ask someone else for
taking over it?

I am working on my tickets (as you can see from commit logs and ticket updates). One ticket takes time to do a full check each time, and for another I am reading before I can fix it. So it's best if I put another ticket that I _can_ fix to review before I do that so that in a week, i would have fixed some things.

I understand it for the longer check one, but starting another ticket
while you're "reading" (not sure what you mean by this though -
something like reading RFCs or something to understand the issue
better?) doesn't make much sense to me. If you need to read for some
ticket, I'd suggest completing the reading first, finishing the
corresponding ticket, and then starting a new one; or if you think the
reading could take too long, release the ticket so that someone else
(who has possibly sufficient background to take over it without doing
extra reading) can complete it.

The point is that, if one starts doing a new task while leaving
something that same person owns in the review queue, we'll build a
longer backlog in the queue. In general, we try to keep it short,
and, basically, we expect one developer normally owns at most two
tickets, and normally at least one of them is a reviewing task. A
single developer owns four tickets, all as a developer, is quite
unusual.

Also, owning a ticket effectively works as a lock, preventing others
from working on it. So, even if some other developer owns nothing and
can work on a new task exclusively, the locked task cannot be owned.
Consider an extreme case where one developer owns all tickets, pending
everything while "reading" something for them. Then the rest of the
developers cannot do anything unless the "reading" developer unlocks
some of the tickets.

Of course, under specific circumstances it might be possible that the
best choice is for one developer to own 3 or more tickets at the same
time. But at least to me, the current status where you owned
#1704/#1771/#1828 and started a new one is something quite different
from what we generally expect.

I also don't have more than 1 ticket in 'assigned' at any time (except once when I had already finished a ticket and was waiting for check to finish).

In my understanding, "accepted" or "assigned" doesn't mean anything to
us beyond that the ticket is owned by that person (we have these
simply because trac has them). What matters is who owns which ticket,
and how many tickets a single developer owns at one time.

comment:6 Changed 7 years ago by muks

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

Up for review.

comment:7 follow-up: Changed 7 years ago by jelte

  • Owner changed from UnAssigned to muks

I must confess I have not checked whether we also need to catch some more specific exceptions that we know can be raised, but in general this code looks fine.

I do have one suggestion, and that is before we do catch(...), we also catch std::exception by name first, so we can at least print *something* more than 'unknown exception' which is nice to prevent abort, but not useful at all :)

comment:8 Changed 7 years ago by jelte

oh and for the exception we do define ourselves, we should probably have unit tests :) (like the InvalidMessageOperation?)

Last edited 7 years ago by jelte (previous) (diff)

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

  • Owner changed from muks to jelte

Replying to jelte:

I do have one suggestion, and that is before we do catch(...), we also catch std::exception by name first, so we can at least print *something* more than 'unknown exception' which is nice to prevent abort, but not useful at all :)

Good point. :) Fixed.

Replying to jelte:

oh and for the exception we do define ourselves, we should probably have unit tests :) (like the InvalidMessageOperation?)

I have added tests for the non-generic catch()s that we've added (please check). A more thorough set of testcases for every exception thrown by Message would be best done in a new bug. The Message class is not fully covered by tests too.

comment:10 Changed 7 years ago by jelte

  • Owner changed from jelte to muks
  • Total Hours changed from 0 to 0.5

ok, thanks, verified the tests, this can be merged

comment:11 Changed 7 years ago by muks

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

Merged to master:

* b56e5b3 [2004] Add Python unittests for the specialized exceptions that we catch
* c28fd5d [2004] Catch std::exception before ... to print some detail if possible
* 1fdb0ee [2004] Catch exceptions in Python Message class wrapper

Resolving as fixed. Thank you for the review.

Note: See TracTickets for help on using tickets.