Opened 7 years ago

Closed 7 years ago

#2349 closed defect (fixed)

Valgrind issues

Reported by: jelte Owned by: jelte
Priority: medium Milestone: Sprint-20121023
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 5 Add Hours to Ticket: 0
Total Hours: 9.63 Internal?: no

Description

As in #2275 this is a ticket to tackle valgrind issues; just spend the allotted time (5 points) on it, tackling more valgrind issues.

Subtickets

Change History (11)

comment:1 Changed 7 years ago by jelte

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

I suddenly had an idea on how we might get valgrind to ignore death tests (or probably the other way around, skip death tests in valgrind), not entirely sure but worth a try

comment:2 Changed 7 years ago by jelte

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

OK, two potentially controversial changes here, but it does fix all of the remaining valgrind issues on my system.

The first one is that every death test only runs EXPECT_DEATH if it is not running under valgrind; the valgrind headers offer a macro for this, so configure.ac checks if those are available. If so, and if valgrind is indeed running, all EXPECT_DEATH tests are skipped.

The second one makes two classes in lib/resolve/recursive_query.[h] public, but they offer only a destructor (the details are hidden behind Pimpl, and the constructor is only available to RecursiveQuery? objects). The biggest drawback is that it now contains two new's instead of one, and it does not address the fundamental issue of these objects that need to live out of scope without a decent pool manager. In this specific case, the unit tests leave out much of the framework and these objects never delete themselves, so providing a destructor gives the unit tests a chance to clean up.

IMO the lib/resolve code should be changed to get this right, it either needs a pool or a different way to consistently handle upstream outstanding queries, that does not need dynamically allocated object with no scope.

But since we're not supposed to put too much work in the resolver right now, I think that simply providing an interface for the tests to delete them should be enough for now.

comment:3 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:4 Changed 7 years ago by vorner

  • Owner changed from vorner to jelte

Hello

I'm not sure about the thing with RunnigQuery and ForwardQuery. In the case
of tests, RunningQuery is deleted by the test and it deletes the
RunningQueryImpl. But under the normal circumstances, the RunningQueryImpl
deletes itself, but who deletes the RunningQuery itself? That one seems to be
just forgotten.

Also, I know you should not spend more time than the 5 points, but maybe
finishing some things off would be allowed? ;-)

    /// TODO: doc
    RunningQuery* running_query_;
    /// TODO
    RunningQuery* running_query_;

comment:5 Changed 7 years ago by jelte

  • Owner changed from jelte to vorner

Doh, of course. I also realized it could be implemented simpler with a base class (AbstractRunningQuery?); the original implementations can then still be kept in anonymous namespace.

Also updated the forgotten TODO's :)

comment:6 Changed 7 years ago by vorner

  • Owner changed from vorner to jelte

Hello

This looks better. But I wonder if the running_query_ in tests can be now
reused for forward queries too instead of holding them on stack.

comment:7 Changed 7 years ago by jelte

  • Owner changed from jelte to vorner

why yes, yes it could :)

updated

comment:8 Changed 7 years ago by vorner

  • Owner changed from vorner to jelte

Looks good, please merge.

Thanks

comment:9 Changed 7 years ago by vorner

  • Total Hours changed from 0 to 1.63

comment:10 Changed 7 years ago by jelte

  • Total Hours changed from 1.63 to 9.63

Thanks, merged, closing ticket

comment:11 Changed 7 years ago by jelte

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

perhaps it helps if i actually set it to closed.

Note: See TracTickets for help on using tickets.