Opened 9 years ago

Closed 9 years ago

#499 closed enhancement (complete)

TCP (as a client)

Reported by: shane Owned by: stephen
Priority: low Milestone: R-Team-Sprint-20110316
Component: resolver Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 10.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no


TCP (as a client)

  • detect TC bit
  • TCP fetcher


Change History (8)

comment:1 Changed 9 years ago by smann

  • Owner set to smann
  • Status changed from new to accepted

(10:56:24 AM) smann: Does anyone remember more details about ticket #499 (TCP as a client)? Is this just: asio doesn't currently accept incoming TCP connections?
(10:58:07 AM) vorner: smann: no, if you look down intoRunningQuery (I think), it asks remote server over UDP. But if that fails because it is truncated, it should be retried using TCP. And that TCP think that could ask another server doesn't exist, nor the code that would check for the truncation.

comment:2 Changed 9 years ago by stephen

  • Owner changed from smann to stephen
  • Status changed from accepted to assigned

comment:3 Changed 9 years ago by stephen

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

RecursiveQuery should now check for the TC bit and, if set, re-try a fetch to the same server over TCP.

Checked in version is commit 1f682b8172e368d7609aa2f066b778989a7fdf7e. There have been a couple of merges with master in this branch which confuses things - when looking at differences I suggest a suitable start point for the review is commit d4af3712f3987069dffe6ed30919ce0b7bf84699 and ignore anything outside src/lib/asiolink.

comment:4 Changed 9 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:5 Changed 9 years ago by jelte

  • Owner changed from jelte to stephen

I've fixed a few typos in some comments, see commit 0b42ead35810e82a97561d71309283950ca9d5a3

The code looks good and can be merged imo.

I did notice that it doesn't work with when in forwarder mode. This isn't a problem in the TCP code itself but apparently not all flags are copied in forwarder mode (at least, I think the forwarder should send back the truncated packet rather than retrying with tcp), I can create a ticket for that, but I think it may need to be a bit more general, perhaps we can discuss that tomorrow.

Another comment I had, there's a TODO in tcp_socket.h (line 175), about the two octets of tcp data length. I agree this can be done better, but disagree with the suggestion there (to put it in buffer). While asio should really be about sending and receiving data, the buffer should imo be just about storing it, and have no knowledge of protocols :)

comment:6 Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

Thanks for the review, just need a review of some changes that occurred to me over the weekend:

  • Increased size of staging buffer from 4096 to 8192. If the UDP packet truncated were an EDNS0 packet with a typical size of 4096, using the same size for the TCP staging buffer would mean that at least two reads would always be required. Increasing the staging buffer size reduces the number of times that multiple reads are required.
  • None of the data in IOFetchData has an existence independent of the IOFetchData object itself. Therefore there is no need for the "socket" and "remote" elements to be pointed to by a shared_ptr: a scoped_ptr (which has the same destruction semantics without the overhead of a reference count) is sufficient. Also, since the size of the staging array is known at compile time, it can be declared in IOFetchData - there is no need to dynamically allocate it.
  • Extended the TCP unit tests to test a variety of transfer sizes from 0 to 65535.

Concerning the TODO, I agree with the idea that we should just be sending and receiving buffers of data without knowledge of the contents. However, if we can add the generalisation that the data starts two bytes into the buffer and that the first two bytes can be used as scratch space, we can make a useful optimization.

comment:7 Changed 9 years ago by jelte

  • Owner changed from jelte to stephen

Looks good, go ahead and merge.

Regarding that todo; I propose we discuss this with the team

comment:8 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 5.0 to 10.0
  • Resolution set to complete
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.