Opened 9 years ago

Closed 9 years ago

#401 closed enhancement (complete)

Timouts in the recursor

Reported by: vorner Owned by: vorner
Priority: medium Milestone:
Component: recurser Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

I implemented basic support of timeouts in the b10-recurse.

The most important part is in UDPQuery (which asks upstream DNS server a question, will probably be used in some similar form by the dispatch), that can report a timeout now. The RecursiveQuery? can retry few times before giving up.

Currently, it needs a merge with the branch from #389 to be configurable. Furthermore, some of the current code is temporary until the merge is done (the #389 contains a DNSService without any sockets, for example).

The timeouts are configured only statically for now (no computing of average timeout, increasing timeouts when a packet is lost, etc). Do you think it should be done now or some time later?

The proposed changelog entry would be:

[func]	vorner
b10-recurse supports timeouts and retries in forwarder mode.
(trac #401, SVN rTBD)

Code is in branches/vorner-recursor-timeouts, branch point r3392.

Subtickets

Change History (10)

comment:1 Changed 9 years ago by vorner

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

comment:2 Changed 9 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:3 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to vorner

src/lib/asiolink/udpdns.cc
There are no comments describing "Priv" nor any of its attributes. (See below for a comment about the name of the structure.)

src/lib/asiolink/tests/udpdns_unittest.cc
Please check with Shane about the modification to the standard copyright notice (i.e. the change to copyright CZ.nic). I'm not certain what the arrangement was concerning copyright.

Perhaps change the name of the test class from UDPQuery? Although it is in a different namespace, there is a risk of confusion with the asiolink::UDPQuery class.

The standard seems to be to append an underscore to class member variables; this should be considered here.

Test classes and methods should be documented to the same standards as the code being tested (so method and class headers should be added).

In TEST_F(UDPQuery, stop), it is not clear why two calls to service.post() are being made. The check only tests that the callback function is called, and does not seem to distinguish the effects of the two calls.

src/lib/asiolink/internal/udpdns.h
Using a shared_ptr is a good idea to minimise the overhead of copying. However, "Priv" is not a very descriptive name for the structure (I thought it was short for "privilege"). I would suggest something like "UdpQueryData".

The comment describing the private data mentions that the object is copied often. I suggest that the reason it is copied often (i.e. that it is used as the callback object in the asynch_*() functions) be noted.

src/lib/asiolink/asiolink.cc
Class RunningQuery should have some header documentation about its purpose.

comment:4 in reply to: ↑ 3 Changed 9 years ago by vorner

  • Owner changed from vorner to stephen

Replying to stephen:

src/lib/asiolink/tests/udpdns_unittest.cc
Please check with Shane about the modification to the standard copyright notice (i.e. the change to copyright CZ.nic). I'm not certain what the arrangement was concerning copyright.

It was agreed that I write CZ.nic until the agreement is finished, then all the headers will be changed to ISC. I'm not sure if it was already signed or not, I hope I'll get notified once it is.

The standard seems to be to append an underscore to class member variables; this should be considered here.

You mean to object member variables, right?

I originally guessed this marks a private member, not variable and since I had them all public, I didn't give them underscores. But I added them.

Test classes and methods should be documented to the same standards as the code being tested (so method and class headers should be added).

I added comments to them, but not doxygen ones (having interface of tests in the API documentation does not seem useful).

In TEST_F(UDPQuery, stop), it is not clear why two calls to service.post() are being made. The check only tests that the callback function is called, and does not seem to distinguish the effects of the two calls.

I tried to explain it in a comment. The second post is not for query, but for stop() ‒ I test stopping the query both after it got called and before.

src/lib/asiolink/internal/udpdns.h
Using a shared_ptr is a good idea to minimise the overhead of copying. However, "Priv" is not a very descriptive name for the structure (I thought it was short for "privilege"). I would suggest something like "UdpQueryData".

I changed it to PrivateData? (UdpQueryData? seems too much duplicating, as it is inside UDPQuery class already).

Everything else should be done now. Do you think it is clean to sync before merge?

Thank you

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

  • Owner changed from stephen to vorner

It was agreed that I write CZ.nic until the agreement is finished,
then all the headers will be changed to ISC. I'm not sure if it was
already signed or not, I hope I'll get notified once it is.

Fair enough - wasn't aware of that.

You mean to object member variables, right?

I originally guessed this marks a private member, not variable and
since I had them all public, I didn't give them underscores. But I
added them.

I'm not sure about that - I thought it was to distinguish between member variables and local ones.

Test classes and methods should be documented to the same standards as
the code being tested (so method and class headers should be added).

I added comments to them, but not doxygen ones (having interface of tests in the API
documentation does not seem useful).

It's always possible to exclude the test files from the Doxygen documentation (see item 10 in http://www.stack.nl/~dimitri/doxygen/faq.html) if we want to. But otherwise, if you're modifying the code you will need to modify the test code as well so may well want to see the documentation for it. However, leave it as is for now.

In TEST_F(UDPQuery, stop), it is not clear why two calls to service.post()
are being made. The check only tests that the callback function is called,
and does not seem to distinguish the effects of the two calls.

I tried to explain it in a comment. The second post is not for query, but for
stop() ‒ I test stopping the query both after it got called and before.

OK - I got confused by the fact that there is a UDPQuery class in the .cc file and one in the test file - I was looking at the wrong one when following the logic.

Using a shared_ptr is a good idea to minimise the overhead of copying. However,
"Priv" is not a very descriptive name for the structure (I thought it was short
for "privilege"). I would suggest something like "UdpQueryData".

I changed it to PrivateData? (UdpQueryData? seems too much duplicating, as it is
inside UDPQuery class already).

Fine.

Everything else should be done now. Do you think it is clean to sync before merge?

Yes.

comment:6 in reply to: ↑ 5 Changed 9 years ago by vorner

  • Owner changed from vorner to stephen

Replying to stephen:

I added comments to them, but not doxygen ones (having interface of tests in the API
documentation does not seem useful).

It's always possible to exclude the test files from the Doxygen documentation (see item 10 in http://www.stack.nl/~dimitri/doxygen/faq.html) if we want to. But otherwise, if you're modifying the code you will need to modify the test code as well so may well want to see the documentation for it. However, leave it as is for now.

Well, I understand the doxygen as documentation for people using the code from outside. For them, the documentation of tests is useless, since tests aren't part of the public API. On the other hand, for someone modifying the code the documentation is not enough, such person needs to read the code. And then there's not much difference between comments directly in the code and the documentation pulled out to doxygen. The important part of doxygen ‒ indices and links ‒ aren't needed, as the person knows which part of code to look into. But then, maybe I'm wrong and I should ask tomorrow on the call.

Everything else should be done now. Do you think it is clean to sync before merge?

Yes.

It is synced. I added some parts to join the merged changes (configuration of timeouts, bits of logging). Can you have a look at them?

Thanks

comment:7 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to vorner

I've looked at the changes in src/lib/asiolink and src/bin/recurse since r3524. The seem OK, although I did note the following concerning recursor.h and recursor.cc:

recursor.h

  • Methods are documented, but not the class itself. What is its purpose?

recursor.cc

  • RecursorImpl::RecursorImpl should initialize rec_query_ to NULL (instead of "()".
  • Is there a potential failure mode here should querySetup() be called when a query is in progress? Suggest that if querySetup() is called with a non-NULL rec_query_ an exception is thrown.
  • The classes QuestionInserter and SectionInserter seem to related more to code associated with general DNS message manipulation than with the recursor. Should they be in recursor.cc or in the main DNS library?
  • Similarly, MessageAnswer does a general transformation of a message. Although it takes a Recursor* argument to its constructor, it seems that this is never used. I would suggest that the functionality of MessageAnswer is placed in the main DNS library and that the MessageAnswer code just calls it.

On a more general point, documentation: as the resolver progresses, it is getting more difficult to relate the bits to one another. I think we need to take some time to document what we have so far. At minimum a class diagram and an outline of the general logic is required. (In fairness, I think that also needs to include the asiolink classes; there is some documentation in the "doc" directory but it does need to be extended. Most of the time in this review was flitting back and forth between asiolink and recurse trying to work out what method was being called and why.)

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

  • Owner changed from vorner to stephen

Replying to stephen:

recursor.h

  • Methods are documented, but not the class itself. What is its purpose?

Fixed

recursor.cc

  • RecursorImpl::RecursorImpl should initialize rec_query_ to NULL (instead of "()".

Fixed

  • Is there a potential failure mode here should querySetup() be called when a query is in progress? Suggest that if querySetup() is called with a non-NULL rec_query_ an exception is thrown.

Well, it is an error to call querySetup() when queryShutdown() wasn't called before, because it would leak the memory of previous RecursiveQuery? object. I added an assert there (this is not something anyone should ever catch).

Current code expects there are no threads here, because queryShutdown could delete the RecursiveQuery? object while a call to its method would be running. That is problem of more of the code there, that it is not thread safe at all and isn't meant to.

But if used from the same thread, calling delete on it is safe at any time. The sendQuery method just creates an autonomous object waiting for the answer, it is independent of the originating RecursiveQuery?.

  • The classes QuestionInserter and SectionInserter seem to related more to code associated with general DNS message manipulation than with the recursor. Should they be in recursor.cc or in the main DNS library?

They do not seem as some utility commonly used. They just hack around C++ inability to have lambda functions and for_each strange interface. If I wrote the code, I'd use a BOOST_FOREACH or put the classes directly inside the function that uses them. If I saw them in the public interface of the library, I'd be asking why these classes are there, when I can just insert into the data directly.

  • Similarly, MessageAnswer does a general transformation of a message. Although it takes a Recursor* argument to its constructor, it seems that this is never used. I would suggest that the functionality of MessageAnswer is placed in the main DNS library and that the MessageAnswer code just calls it.

Yes, I removed the argument, it was a leftover from some older code. Maybe gcc should warn about private unused member variable as well.

This one inherits from DNSAnswer, which is from asiolink. I guess the dns library should be able to work without asiolink, therefore it shouldn't contain such class.

I'm not definite on these two, but having them locally unless someone else has need for them elsewhere seems to make more sense.

On a more general point, documentation: as the resolver progresses, it is getting more difficult to relate the bits to one another. I think we need to take some time to document what we have so far. At minimum a class diagram and an outline of the general logic is required. (In fairness, I think that also needs to include the asiolink classes; there is some documentation in the "doc" directory but it does need to be extended. Most of the time in this review was flitting back and forth between asiolink and recurse trying to work out what method was being called and why.)

Should this be part of this ticket?

Not that I wouldn't agree that there's need of documentation (it took me a while to read and remember how the code works and I'll forget it soon again), but I think it should go either into #327 or into a separate task. Is it OK if I just create a ticket or put this into the rteam backlog?

comment:9 Changed 9 years ago by stephen

  • Owner changed from stephen to vorner
  • The classes QuestionInserter and SectionInserter seem to related more to code associated

with general DNS message manipulation than with the recursor. Should they be in
recursor.cc or in the main DNS library?

They do not seem as some utility commonly used. They just hack around C++ inability to have
lambda functions and for_each strange interface. If I wrote the code, I'd use a
BOOST_FOREACH or put the classes directly inside the function that uses them. If I saw
them in the public interface of the library, I'd be asking why these classes are there,
when I can just insert into the data directly.

Fair enough. But a comment to that effect would be useful.

Also, SectionInserter::sign_ appears not to be used.

  • Similarly, MessageAnswer does a general transformation of a message. Although it takes a

Recursor* argument to its constructor, it seems that this is never used. I would suggest
that the functionality of MessageAnswer is placed in the main DNS library and that the
MessageAnswer code just calls it.

:
This one inherits from DNSAnswer, which is from asiolink. I guess the dns library should
be able to work without asiolink, therefore it shouldn't contain such class.

It wasn't so much that, it's just that the processing - copying a lot of the input query into the output response - would also seem to more about processing a DNS message in general rather than be recursor-specific. As such, I would think it would have a lot in common with the processing in the authoritative server and so could be shared code. But leave it for now.

Not that I wouldn't agree that there's need of documentation (it took me a while to read
and remember how the code works and I'll forget it soon again), but I think it should go
either into #327 or into a separate task. Is it OK if I just create a ticket or put this
into the rteam backlog?

I've added it as a task to the R-Team backlog.

comment:10 Changed 9 years ago by vorner

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

Ok, comment added and merged. Closing the ticket.

Note: See TracTickets for help on using tickets.