Opened 7 years ago

Closed 7 years ago

#2160 closed defect (fixed)

b10-auth should clear Message at the end of request handling

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

Description (last modified by jinmei)

In the current code, the Message object keeps all RRsets stored
in the response, and they are cleared at the beginning of next request
handling.

With the revised version of in-memory data source this is not good
because the RRsets would have a reference to internal data structures
of the data source (tree node, RdataSet object, etc), and if the
in-memory image is replaced due to like reload between the two
requests, the Message clear operation will lead to some undesirable
behavior.

We should revise the code so that the Message is always cleared
at the end of request handling (note: it shouldn't affect sending the
response data once rendering is completed). We'd probably extend
the existing RendererHolder helper class to support both the
renderer and the message.

When this is done, I believe we can remove the workaround in
~AuthSrvText().

Subtickets

Change History (15)

comment:1 Changed 7 years ago by jinmei

  • Description modified (diff)

comment:2 Changed 7 years ago by jelte

  • Milestone set to Sprint-20120821

comment:3 Changed 7 years ago by kevin_tes

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

comment:4 Changed 7 years ago by kevin_tes

Kevin take this task.

comment:5 Changed 7 years ago by jelte

  • Owner changed from kevin_tes to jelte
  • Status changed from accepted to assigned

Sorry kevin, I don't know if you had done any work on this already, but since you haven't responded yet, I'm stealing this ticket :)

comment:6 follow-up: Changed 7 years ago by jelte

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

Ready for review, I did create a separate Holder class for the message, and put it at a slightly different location (operator() instead of in processMessage() itself), mostly so that the tests can still inspect the message is processed correctly.

comment:7 in reply to: ↑ 6 Changed 7 years ago by jinmei

Replying to jelte:

Ready for review, I did create a separate Holder class for the message, and put it at a slightly different location (operator() instead of in processMessage() itself), mostly so that the tests can still inspect the message is processed correctly.

  • I first thought I'd like to test the case where processMessage() throws. The current test would pass with a more straightforward version of cleanup:
            server_->processMessage(io_message, *message, *buffer, server);
            message->clear(Message::PARSE);
    
    (but this one isn't fully exception safe). But then I notice most of the exceptions are caught in processMessage(). And, if an uncaught exception leaks to the operator(), that would kill b10-auth anyway, so the cleanup effort will be moot. At this point I'm not sure what to do, but if it's not difficult I'd still test a case where processMessage() throws.
  • not really related to this ticket, but with this cleanup we now do Message::clear() twice, both in the operator() and in SyncUDPServer::handleRead(). The latter is effectively waste. For a bit longer term, I think we should rather remove the Message object from the SyncUDPServer implementation.
  • Just checking: do we need a changelog entry? It's a mostly invisible internal change, so we probably don't need it, but maybe we do.

comment:8 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jelte

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

  • Owner changed from jelte to jinmei

It would appear that processMessage() is too robust ;) I couldn't really come up with any way to make it throw without making some existing class methods virtual, so tbh I think it is not worth the trouble.

I did remove the one call to clear() from SyncUDPServer, but left that class as it was for the rest (removing the message there is nontrivial and indeed not really related)

I do not think this needs a changelog message.

comment:10 in reply to: ↑ 9 Changed 7 years ago by jinmei

Replying to jelte:

It would appear that processMessage() is too robust ;) I couldn't really come up with any way to make it throw without making some existing class methods virtual, so tbh I think it is not worth the trouble.

Okay.

I do not think this needs a changelog message.

Okay.

I did remove the one call to clear() from SyncUDPServer, but left that class as it was for the rest (removing the message there is nontrivial and indeed not really related)

If you go this further, I'd like to create a ticket to do it. It
should at least unify the consistent behavior about whether to pass
a Message object from the XXXServer in the first place, and if the
conclusion is they don't pass it, revise the code.

And, then, note it here:

    // Make sure the buffers are fresh
    output_buffer_->clear();
    answer_->clear(isc::dns::Message::RENDER);

referring to that ticket. Otherwise it wouldn't be clear who should
clear the message and when.

comment:11 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:12 Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

hmm, looking at the XXX servers, all of them do it, so in that sense they are unified (tcp_ and udp_ do initialize them in a different place). So is it only about cleaning them? But maybe I'm just misinterpreting your request here.

comment:13 Changed 7 years ago by jinmei

I created a ticket I intended (#2239). And my suggested revised
comment to SyncUDPServer::handleRead is as follows:

    // Make sure the buffers are fresh.  Note that we don't touch query_
    // because it's supposed to be cleared in lookup_callback_.  We should
    // eventually even remove this member variable (and remove it from
    // the lookup_callback_ interface, but until then, any callback
    // implementation should be careful that it's the responsibility of
    // the callback implementation.  See also #2239).
    output_buffer_->clear();
    answer_->clear(isc::dns::Message::RENDER);

If the ticket and the comment make sense to you, please merge the
branch with the added comment.

comment:14 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:15 Changed 7 years ago by jelte

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

aaah, ok.

Added the comment and merged the branch, thanks.

Note: See TracTickets for help on using tickets.