repo-ng

We also observed the same issue.
We found that the error is raised by the RepoCommandReponse's wireDecode: the type is supposed to be 207 but which in fact is 101.
101 is the type number of the parent class of RepoCommandReponse -- ControlReponse, so the problem is actually caused by the class inheritance.

Yufeng Zhang from UCLA has been assigned to fix on this error.

Additionally, I have found the following problems

1. ndnputfile command doesn't append EndBlockID by default, which is causing command validation failure, and so the write-handle insertion is failing.
   starting here, https://github.com/named-data/repo-ng/blob/master/src/handles/write-handle.cpp#L48
   and finally here https://github.com/named-data/repo-ng/blob/master/src/handles/command-base-handle.hpp#L68, with following error messages "EndBlockId is required but missing"

2. When a response is created inside processSegmentedInsertCommand (write-hanldle.cpp #L256, done(response) is called ----- this goes all the way to ndn-cxx Dispatcher::processControlCommandInterest. Now, interest component is assigned to cxx parameter (shared_ptr).

dispatcher.cpp #L175

// /<prefix>/<relPrefix>/<parameters>
  size_t parametersLoc = prefix.size() + relPrefix.size();
  const name::Component& pc = interest.getName().get(parametersLoc);

  shared_ptr<ControlParameters> parameters;
  try {
    parameters = parser(pc);
  }
  catch (const tlv::Error&) {
    return;
  }

this creates a mismatch. And thus, everything gets messed up.

Probably, ndn-cxx -- Dispatcher::processControlCommandInterest(...) function should be Template?

Yufeng Zhang from UCLA has been assigned to fix on this error.

Please have this developer join this project and then update Assignee field.

Saurab volunteered to take this.

Current proposed solution: make wireEncode and wireDecode virtual in ControlResponse command.
Seems unacceptable due to being the only wire* functions to be virtual in the code - which is unexpected.

Write-handle calls CommandContinuation (done) in dispatcher through the handler. The done takes in a ControlResponse.
Repo-ng provides the done function a RepoCommandResponse (sub-class of ControlResponse). Since wireEncode is not virtual, when Dispatcher calls wireEncode it is called on the parent class. Why the child wireEncode is not called could be because of dynamic_cast in CommandBaseHandle::validateParameters (Saurab found this).

The wireEncode in ControlResponse encodes the body within it. The body is set to be the wireEncode of the RepoCommandResponse. So another solution could be to change wireDecode of RepoCommandResponse to be process the body of the response.

Ashlesh Gawande wrote:

Write-handle calls CommandContinuation (done) in dispatcher through the handler. The done takes in a ControlResponse.
Repo-ng provides the done function a RepoCommandResponse (sub-class of ControlResponse). Since wireEncode is not virtual, when Dispatcher calls wireEncode it is called on the parent class.

Correct.

Why the child wireEncode is not called could be because of dynamic_cast in CommandBaseHandle::validateParameters (Saurab found this).

Not following this part. What has validateParameters got to do with ControlResponse?

The wireEncode in ControlResponse encodes the body within it. The body is set to be the wireEncode of the RepoCommandResponse. So another solution could be to change wireDecode of RepoCommandResponse to be process the body of the response.

But this encoding violates the protocol, doesn't it?

Davide Pesavento wrote:

Ashlesh Gawande wrote:

Write-handle calls CommandContinuation (done) in dispatcher through the handler. The done takes in a ControlResponse.
Repo-ng provides the done function a RepoCommandResponse (sub-class of ControlResponse). Since wireEncode is not virtual, when Dispatcher calls wireEncode it is called on the parent class.

Correct.

Why the child wireEncode is not called could be because of dynamic_cast in CommandBaseHandle::validateParameters (Saurab found this).

Not following this part. What has validateParameters got to do with ControlResponse?

Please forget about this, I was lost in something else.

The wireEncode in ControlResponse encodes the body within it. The body is set to be the wireEncode of the RepoCommandResponse. So another solution could be to change wireDecode of RepoCommandResponse to be process the body of the response.

But this encoding violates the protocol, doesn't it?

Yes, this encoding is violating the protocol.

tl;dr;

Here is a scenario of how the problem is occurring.

class Base
{
  public:
  void aa() const
  {
    cout << "this is from parent" << endl;
  }
};
class Derived: public Base {
  public:
  void aa() const
  {
    cout << "This is from child" << endl;
  }
};
static void f (const Base* a)
{
  a->aa();
}

int main () {
   Derived* d = new Derived;
   f(d);
   return 0;
}

The function "f" which is similar to the dispatcher, when get derived class pointer as the base class pointer will always call base class method. Dispatcher here is falsely assuming that the derived class method will be called, and this is creating the problem. This problem will persist for any other similar implementation. But unfortunately, I don't see any other classes inheriting ControlResponse except repo.

Actual code flow:
https://github.com/named-data/repo-ng/blob/master/src/handles/write-handle.cpp#L127 (sends RepoCommandResponse:: response) --- > https://github.com/named-data/ndn-cxx/blob/master/ndn-cxx/mgmt/dispatcher.cpp#L202 (gets as resp) ----> https://github.com/named-data/ndn-cxx/blob/master/ndn-cxx/mgmt/dispatcher.cpp#L218 (and here resp.wireEncode() calls base method).

And I don't know how to solve this problem without making the base method virtual.

Saurab Dulal wrote:

Dispatcher here is falsely assuming that the derived class method will be called, and this is creating the problem.

Dispatcher is not assuming anything. It was simply not designed to support different "response" types, and ControlResponse was not designed to support inheritance. The mistake here was using this machinery in repo-ng with a custom type used as response.

But unfortunately, I don't see any other classes inheriting ControlResponse except repo.

Because ControlResponse was not designed to be inherited from.

And I don't know how to solve this problem without making the base method virtual.

Adding more template parameters will probably solve this, but it could get messy...

Davide Pesavento wrote:

Ashlesh Gawande wrote:

Write-handle calls CommandContinuation (done) in dispatcher through the handler. The done takes in a ControlResponse.
Repo-ng provides the done function a RepoCommandResponse (sub-class of ControlResponse). Since wireEncode is not virtual, when Dispatcher calls wireEncode it is called on the parent class.

Correct.

I quite don't understand why the wire E/D are virtual on the control-parameters? It was argued earlier on the NFD call/gerrit that encodable types cannot be virtual.
https://github.com/named-data/ndn-cxx/blob/master/ndn-cxx/mgmt/control-parameters.hpp

Saurab Dulal wrote:

I quite don't understand why the wire E/D are virtual on the control-parameters? It was argued earlier on the NFD call/gerrit that encodable types cannot be virtual.
https://github.com/named-data/ndn-cxx/blob/master/ndn-cxx/mgmt/control-parameters.hpp

That may simply be a historical mistake. As I mentioned in the call, similar mistakes have been made with other classes.

That being said, it's worth noting that mgmt::ControlParameters is just an interface, not a complete type, it doesn't define any fields or anything like that, and concrete classes are supposed to derive directly from the abstract interface. That is quite different from the ControlResponse situation and not as bad. We haven't considered this approach during the call, but I think I would not disagree if you wanted to follow it (assuming it's feasible), and it may be simpler than the template-based solution. Essentially you'd have an interface class for ControlResponse, and both NFD's ControlResponse and repo-ng's RepoCommandResponse would directly derive from it.

Actually, you'd probably still have to add an additional template parameter to some Dispatcher methods, in order to tell it what concrete class should be instantiated for the response...