Opened 10 years ago

Closed 10 years ago

#310 closed enhancement (fixed)

proposal: constify lib/cc API more appropriately

Reported by: jinmei Owned by: jinmei
Priority: low Milestone: y2 6 month milestone
Component: configuration 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: 8.0 Internal?: no

Description

I've been noticing (but haven't had time to address) that constness of isc::data::ElementPtr? could be improved in many points:

  • at the very least, I suspect the usage of 'const ElementPtr' is quite unlikely to be what was actually intended. I guess it was intended to be a shared_ptr version of 'const Element*', which is actually be 'shared_ptr<const Element>' (which I'd name 'ConstElementPtr'), not 'const shared_ptr<Element>'.
  • assuming the first point is correct, if we change 'const ElementPtr' to 'ConstElementPtr', it would be clearer that many of the usage is actually read-only and many other ElementPtr can be changed to ConstElementPtr
  • after doing this, we can change many method functions to const member functions.

Without these changes the code can work correctly, but since this API is quite complicated I think we can greatly benefit from making things const as much as possible. One typical benefit is that we could avoid introducing a bug that the content of an ElementPtr is magically modified at some deep level of API.

I've finally managed to make these changes, and am going to make a separate branch to get the proposed change reviewed.

Subtickets

Change History (4)

comment:1 Changed 10 years ago by jinmei

branches/trac310 is ready for review.

I believe Jelte is the best person to review it, so I'm giving it to him. Please speak up if you are too busy or if you can have a suggested alternative reviewer.

I know the diff is large, but most of the changes are trivial type change from non-const to const (i.e. safer move), and the fact it compiles and passes tests should be sufficient to check the correctness.

Here are notes on major non trivial points in the diff. Note that a few of them are a fix to bugs of the current code, which should be fixed whether or not we adopt this branch. See 'related notes" on updateRemoteConfig() and the bullet for ListElement::set().

  • there are cases where I needed to do more than simple type conversion. Most of them are in ccsession.cc. Relatively trivial ones are changes like that for createAnswer():
    -    ElementPtr answer_content = answer->get("result");
    +    ElementPtr answer_content = Element::createList();
         answer_content->add(Element::create(rcode));
         answer_content->add(arg);
    +    answer->set("result", answer_content);
    
    that is, since get() is now a const member function that returns a ConstElementPtr object, we cannot use its result to override the content. Instead, I begain with a new empty list, built the answer in it, and store it in the "answer" pointer. This makes the whole process a bit more complicated than the original code, but I believe the safety benefit from the fact that get() returns a non-writable object is a good tradeoff.
  • Likewise, in ModuleCCSession::addRemoteConfig() I made this change:
    -    ElementPtr new_config = parseAnswer(rcode, answer);
    -    if (rcode == 0) {
    -        rmod_config.setLocalConfig(new_config);
    +    int rcode;
    +    ConstElementPtr new_config = parseAnswer(rcode, answer);
    +    if (rcode == 0 && new_config) {
    +        ElementPtr local_config = rmod_config.getLocalConfig();
    +        isc::data::merge(local_config, new_config);
    +        rmod_config.setLocalConfig(local_config);
    
    new_config is now a ConstElementPtr object and cannot be set as a local config (which is supposed to be modifiable and cannot be a const). So I take the "get local, merge, then set", thereby effectively making a local copy of "new_config". one possible concern here is a copy overhead, and other possible issue is that if the intent is to actually share "new_config" with some others, making a copy breaks that assumption. I personally saw these okay, but am open to other opinions.
  • a related note: I added a condition before copying the remote config, that is, confirming new_config isn't empty. I believe this additional check is necessary regardless of whether we adopt this set of const-related changes because otherwise a subsequent operation of updateRemoteConfig() would trigger an exception (at that point the stored config can be empty, which breaks an assumption of data::merge())
  • another related note: with this change, the very last addRemoteConfig() of CCSessionTest::remoteConfig() would trigger an exception. the current implementation does nothing (not even EXPECT_EQ, etc) on this case, but I believe it should actually trigger an exception, and I modified the test case accordingly.
  • (again) Likewise, I ended up introducing a different version of data::removeIdentical() to make a copy for ModuleCCSession::handleConfigUpdate(). As a result, this is actually the only version of this function that is used. So, if this approach makes sense, we might even obsolete the other version.
  • Not directly related to these set of changes, but I modified argument type of some method/functions from 'const ElementPtr' to 'const Element&'. These include operator<< and operator== defined in lib/cc/data.cc. This change is two-fold:
    • first, 'const ElementPtr' is unlikely to be what was intended anyway (which is one main subject of this ticket)
    • second, even if we change it to ConstElementPtr, this interface is still questionable. (in my understanding) shared pointer classes are generally expected to behave like bare pointers. so, if we define operator== for them, the naturally expected behavior would be to compare the stored pointers (actually, shared_ptr defines this operator that way). but our customized version of operator== breaks expectation, which would lead to counter intuitive behaviors. I was not sure if this overriding was intentional, but didn't think so, because these are basically only used internally and in tests, and none of the usage didn't seem to rely on this overriding.
  • due to this change I needed to adjust some test cases and internal code of data.cc. but they should be trivial.
  • I changed the argument type of some functions from (Const)ElementPtr& to (Const)ElementPtr when possible. The former allows the called function to override the pointer itself (not the object that the pointer points to) and can be dangerer than the latter in general. In most of usage, however, this doesn't seem to be the intended interface.
  • there's an off-by-one bug in ListElement::set(). I fixed it in this branch and added related test cases to Element::ListElement tests. This bug needs to be fixed anyway.
  • I tried to avoid introducing unrelated changes to this branch, but there are still some unrelated cleanups. Most of them should be trivial. One possibly unclear cleanup would be the removal of FakeSession::connect()/sendmsg(). I removed them simply because they were unused.

comment:2 Changed 10 years ago by jinmei

  • Add Hours to Ticket changed from 0.0 to 5.0
  • Owner changed from jinmei to jelte
  • Status changed from new to reviewing
  • Total Hours changed from 0.0 to 5.0

comment:3 follow-up: Changed 10 years ago by jelte

  • Add Hours to Ticket changed from 0.0 to 3.0
  • Owner changed from jelte to jinmei
  • Total Hours changed from 5.0 to 8.0

Minor style issue discussed on jabber, committed fix in r2800

I disagree with the statement that the Element API is complicated (unless you are talking about the ccsession/modulespec api) btw :p

Anyhew, apart from the minor style thing I have no substantial problems with these changes.

Two things though; The operator overrides using ElementPtr? instead of Element were intentional, since users of this API should only be working with Ptr's anyway, and to me it looks better to use them directly (compare the changed test cases for instance). However I don't have a strong opinion on this.

Secondly, I do worry a bit about the now-introduced copy overhead (which was why the merge etc were done in-place). But I acknowledge the added safety. So for now I think this is ok. At some point in the future we might need to consider having non-const versions of some methods, but we'd need to profile those cases when they arise, and for the near future I see no problems with them, and

I think this is ok to commit.

comment:4 in reply to: ↑ 3 Changed 10 years ago by jinmei

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

Replying to jelte:

Thanks for the review.

I disagree with the statement that the Element API is complicated (unless you are talking about the ccsession/modulespec api) btw :p

Okay, I admit "complicated" is often subjective:-)

Two things though; The operator overrides using ElementPtr? instead of Element were intentional, since users of this API should only be working with Ptr's anyway, and to me it looks better to use them directly (compare the changed test cases for instance). However I don't have a strong opinion on this.

Hmm, (noting it's not a strong opinon of yours) if you still like the previous approach I don't oppose to overriding it.

Just to be a bit clearer about my comment, here's one specific problematic case with the original definition that I can think of:

    void some_func(Element* a, Element* b) {
        if (a == b) {
            a->setValue(10);
        }
    }

from this code, we'd expect b's value is also modified to '10', and it's quite likely to be the case. I'd expect the same with the shared pointer version of code:

    void some_func_shptr(ElementPtr a, ElementPtr b) {
        if (a == b) {
            a->setValue(10);
        }
    }

but this is not always the case with the previous definition of operator==, and that's what I meant by "counter intuitive behaviors" in my original comment.

Secondly, I do worry a bit about the now-introduced copy overhead (which was why the merge etc were done in-place). But I acknowledge the added safety. So for now I think this is ok. At some point in the future we might need to consider having non-const versions of some methods, but we'd need to profile those cases when they arise, and for the near future I see no problems with them,

Fair enough. I'm okay with revisiting this part if/when the copy overhead becomes a significant performance bottleneck.

One more point: I've noticed ConfigData::getModuleSpec() should probably return 'const ModuleSpec?&' instead of 'const ModuleSpec?' and made that change (at the very least returning a 'const object' is meaningless anyway). With the current definition of ConfigData? these two versions should have exactly the same effect, but if the original intent was in fact to return a copy of the object, please make that change separately.

I've merged trac #310 to trunk, and I'm going to close this ticket.

Note: See TracTickets for help on using tickets.