Opened 9 years ago

Closed 9 years ago

#383 closed enhancement (fixed)

Generalize IOService constructor

Reported by: jelte Owned by: each
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

Currently (that is, in branch327 right now), the IOService constructor takes three Provider arguments.

Two of these are highly DNS-packet-specific ('lookup', and 'answer').

We should generalize this so modules that do not necessarily handle DNS queries and answers can also use our IOService class.

Subtickets

Change History (6)

comment:1 in reply to: ↑ description Changed 9 years ago by each

We should generalize this so modules that do not necessarily handle DNS queries and answers can also use our IOService class.

I agree and would like to expand on this a bit.

The current model (in #327) is that there's an abstract DNSServer class, subclassed as UDPServer and TCPServer, instances of which which are created by the IOServiceImpl constructor. Those DNSServer instances care about the providers; IOService doesn't--but because IOService has to create them, we have to pass the providers to the IOService constructor.

I suggest splitting this into two parts: IOService becomes a simple wrapper around the asio::io_service class, and a new class, DNSService, creates the server objects. This is a pretty minimal change; where we used to have:

    IOService io_service(*port, *address, checkin, lookup, answer);
    ...
    io_service.run()

...we would now use:

    IOService io_service;
    DNSService dns_service(io_service, *address, *port, checkin, lookup, answer);
    ...
    io_service.run();

comment:2 follow-up: Changed 9 years ago by jelte

Ok, I made the proposed change in r3305 and r3306.

IOService still uses that Impl pattern (so other code does not have to (indirectly) include asio/io_service.hpp), and I now wonder whether DNSService (which is mostly the old IOService without the IOService-specific function (i.e. run(), stop() etc.) still needs to do the same.

It doesn't for the sake op asio::io_service, but it currently uses those internal/*dns.h headers, which would then need to become public. Should we leave this as it is?

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

Replying to jelte:

IOService still uses that Impl pattern (so other code does not have to (indirectly) include asio/io_service.hpp)

Right. If we de-pimpl IOService, we might as well get rid of it and just use asio::io_service.

and I now wonder whether DNSService (which is mostly the old IOService without the IOService-specific function (i.e. run(), stop() etc.) still needs to do the same. It doesn't for the sake op asio::io_service, but it currently uses those internal/*dns.h headers, which would then need to become public. Should we leave this as it is?

I think we should keep it for that last reason--keeping tcpdns.h and udpdns.h hidden.

Review notes:

+    /// These are currently very specific to the authoritative server
+    /// implementation.

I hadn't realized this comment was still there; it's not correct anymore.

Not sure why DNSService returns a native asio::io_service instead of an IOService, but it's fine with me either way. (If it changes, though, it ought to be renamed getIOService(), both for clarity and for style compliance. I assume IOService::get_io_service() was named the way it was because proper capitalization would be misleading about its return type; anyway, that's why I left it that way.)

asiolink_unittest.cc:

+        if (dns_service_ != NULL) {
+            delete dns_service_;
         }

Stephen mentioned in his review of #327 that checking for null around a delete isn't necessary; I forgot to fix that.

main.cc:

+        io_service = new IOService();

io_service could be allocated on the stack now. The only reason it wasn't before was that it needed to be instantiated down inside the if statement.

recursor_unittest.cc:
Changes are fine, but will also need to be made in auth_unittest.cc; I don't see that in the diff.

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

  • Owner changed from jelte to each
  • Status changed from new to reviewing

Replying to each:

Review notes:

+    /// These are currently very specific to the authoritative server
+    /// implementation.

I hadn't realized this comment was still there; it's not correct anymore.

Removed. While I was at it, I also added \param documentation in asio_link.h

Not sure why DNSService returns a native asio::io_service instead of an IOService, but it's fine with me either way. (If it changes, though, it ought to be renamed getIOService(), both for clarity and for style compliance. I assume IOService::get_io_service() was named the way it was because proper capitalization would be misleading about its return type; anyway, that's why I left it that way.)

asiolink_unittest.cc:

+        if (dns_service_ != NULL) {
+            delete dns_service_;
         }

Hmm. At this moment I think the only reason this function is used is to get to the native one in the end, which is why right now I made it pass that through directly. I'm inclined to leave it that way for now. Should be a trivial change if anyone disagrees.

Stephen mentioned in his review of #327 that checking for null around a delete isn't necessary; I forgot to fix that.

doh, I knew that. Still some C habits shining through :)

main.cc:

+        io_service = new IOService();

io_service could be allocated on the stack now. The only reason it wasn't before was that it needed to be instantiated down inside the if statement.

Ack. Still needs to be global though, unfortunately.

recursor_unittest.cc:
Changes are fine, but will also need to be made in auth_unittest.cc; I don't see that in the diff.

IOService isn't used directly in those tests, so there wasn't much to change...

Review comments addressed in r3312

comment:5 in reply to: ↑ 4 Changed 9 years ago by each

Replying to jelte:

IOService isn't used directly in those tests, so there wasn't much to change...

Huh, you're right... my brain was stuck in an earlier version of the code, when it was necessary to pass the IOService to processMessage(). I forgot I'd removed that parameter.

That being the case, both the IOService and the DNSService members can be deleted from recursor_unittest.cc.

Review comments addressed in r3312

They look fine to me.

comment:6 Changed 9 years ago by jelte

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

Ok, removed them from the test.

Merged the branch back into branches/trac327 and removed it. Closing ticket.

Note: See TracTickets for help on using tickets.