Opened 8 years ago

Closed 6 years ago

#2265 closed task (wontfix)

cleanup: remove Renderer::clear() calls

Reported by: jelte Owned by:
Priority: medium Milestone: Remaining BIND10 tickets
Component: Unclassified Version: bind10-old
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: 0 Internal?: no

Description

In 1357, we found that essentially it is always mandatory to call Renderer::clear() before Message::toWire(), so we put it in there at the start. Not doing clear() overwrites the header as the start of the already rendered message, leaving the new header uninitialized, and it creates bad compressed domain names in the new packets. Apart from that, truncation may go wrong, and in case of truncation, the renderer is cleared anyway (So there isn't even a reliable contract on whether or not the renderer is cleared).

There are however probably quite a few now unnecessary calls to Renderer::clear() that could now be cleaned up.

Subtickets

Change History (5)

comment:1 Changed 8 years ago by jinmei

What's the goal of this ticket? Remove unnecessary calls to
MessageRenderer.to_wire from xfrout? Remove such calls throughout
the source code? Is it Python or C++ or both (the :: notation seems
to indicate C++, but #1357 is for xfrout, which is written in Python)?

BTW, if it's for python, I've been thinking we might provide an
enhancement to Message.to_wire() so it can use an internal
MessageRenderer and directly returns a byte sequence. Then the code
will be much more simplified like this:

    s.send(msg.to_wire(compress_mode=CASE_SENSITIVE,
                       truncation_limit=65535,
                       tsig_ctx=if_using_tsig))

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

nono, no change to toWire() calls, only calls to clear().

in #1357, we found a problem: if clear() is not called before every single call to toWire(), bad things happen. So I porposed to actually do clear() in Message::toWire() itself. And if that get's through, we suddenly have a lot of unnecessary calls to clear() (everywhere, but in theory easily greppable).

Your suggestion seems fine though

comment:3 in reply to: ↑ 2 Changed 8 years ago by jinmei

Replying to jelte:

nono, no change to toWire() calls, only calls to clear().

Ah, I actually meant clear() but mistyped it as toWire().

in #1357, we found a problem: if clear() is not called before every single call to toWire(), bad things happen. So I porposed to actually do clear() in Message::toWire() itself. And if that get's through, we suddenly have a lot of unnecessary calls to clear() (everywhere, but in theory easily greppable).

Ah, okay. That may make sense in some sense. On the other hand, I
guess we should still encourage the application to clear the renderer
once it's done with it. That's partly because the renderer could hold
a relatively large amount of temporary resource and until the next
call to Message::toWire(), which is generally unpredictable. It
could be more serious if and when we introduce an optimized renderer
implementation for the in-memory data source that internally maintains
some reference to the data source related objects that may be
invalidated after the query processing.

But then if the application follows the cleanup convention, the call
to clear() within Message::toWire() will be redundant, and it will
possibly be a non-negligible overhead for performance sensitive
applications like b10-auth.

So I think we need some better middleground. Based on this discussion
with my older suggestion, I think we could do:

  • have Message objects maintain their own renderer and provide a read-only accessor to it.
  • introduce another version of Message::toWire(). It doesn't take a renderer, but dumps the wire-format data to the internally maintained renderer. the application can then retrieve the data via the accessor method (we can make the python version more convenient as I suggested in my previous comment)

Normal (non performance sensitive) applications will use the latter
version. Then these apps don't have to create/maintain a renderer
themselves. Only performance sensitive apps will use the interface
that takes an external renderer. Such apps are responsible for
clearing the renderer after they are done with it.

comment:4 Changed 6 years ago by tomek

  • Milestone set to Remaining BIND10 tickets

comment:5 Changed 6 years ago by tomek

  • Resolution set to wontfix
  • Status changed from new to closed
  • Version set to old-bind10

This issue is related to bind10 code that is no longer part of Kea.

If you are interested in BIND10/Bundy framework or its DNS components,
please check http://bundy-dns.de.

Closing ticket.

Note: See TracTickets for help on using tickets.