NFD

Please update Description (not Notes) with steps to reproduce, expected, and actual behavior.

I'm working with the test case to reproduce/test the issue.

Nfd and Nrd types are not thread safe. The snippet calls their destructors in another thread, which could lead to undefined behavior.

This is just snippet. The actual test case has a little bit of thread safety handling. The test case reproduces the problem and is reference implementation. The snippet just to illustrate the issue.

Technically, this task is not blocked by nfd/nrd merge. It is only blocked by #2516

If the bug report is illustrated with code snippet, the snippet must be correct. Alternatively, describe the steps in text, and the test case in the solution should match this text.

Junxiao, let's be productive. I don't want to waste time on formal things. My opinion is that the snippet and the test case achieve their different goals.

It's not "formal things". A Bug report must allow others to understand what's the problem. I cannot understand the problem from this snippet, thus it needs revising.

I don't understand the significance of this problem. Nfd never needs to restart within a process. If it crashes, the process should exit.

This approach does not work when nfd needs to be started again within the same process---whenever global io_service is started again, it will fire up a few handlers that point to deallocated memory.

Why would NFD need to be started again within the same process? Just quit the process and let the monitor to restart it.

I don't have an answer of "why". It is more question of purity and "correctness": I would like to have Nrd/Nfd/Forwarder classes to do proper cleanup in a proper order, which gives ability to re-start NFD within the same process if somebody ever wants to do that.

I don't have an answer of "why". It is more question of purity and "correctness": I would like to have Nrd/Nfd/Forwarder classes to do proper cleanup in a proper order, which gives ability to re-start NFD within the same process if somebody ever wants to do that.

I won't agree with this change until there's a use case.

The easy solution is: let Nfd type own the io_service instance, which is destroyed whenever Nfd is destructed. Same goes for Nrd type.

What's a point of you blocking the change? There is no functional change and it just cleans up the order things are destructed. I can come up with any work-around, this change is purely for conceptual correctness.

I won't agree with this Change until there's a use case. Without a use case, this is "unnecessary changes".

Even without the "in-process restart", it makes sense to cleanup in destructors... I'd tend to agree with Alex here.

Then update the issue title and description to get rid of "restart".

Davide's comment on gerrit:

Shouldn't be the channels' responsibility to close the faces though? The channels create them, I'd expect the channels to also destroy them. FaceTable is just a passive observer in this.

Junxiao's comment on gerrit:

Yes

While I'm in principle agree that FaceTable may not be a good place to "own" faces. Channels is not a good place either. First, channels are not the only place where face is created. Some faces are channel-less (e.g., multicast faces) and some faces are completely channel- and factory-less (InternalFace). The only place that has a complete knowledge about all faces is the forwarder itself and in particular FaceTable. Therefore, I would argue that FaceTable is the owner of faces and channels are just observers overseeing faces that they created.

Channels or factories it's not too important... the point is that "normal" faces are owned by channels (or factories), channels are owned by factories, factories are owned by the FaceManager. InternalFace and NullFace are constructed by Nfd and should be closed by it.

This reminds me of a thought I had a while ago, see #2300 note 8.

Also I have a question about this task description: if everything is destructed, why are there "dangling handlers"? Handlers for what specifically?

In some environments (specifically Android app), it may be necessary (at least needed for now) to start/stop NFD multiple times within the same process. This requires that Nrd/Nfd destructors properly cancel everything that was posted on io_service without requiring to stop io_service.

Why can't Android app destroy and recreate io_service?

I could agree with using weak_ptr instead of shared_ptr in many cases. However, this would require a lot of changes and I really don't want to do them any time soon.

As for removal and dangling stuff (this may not be entirely correct in the description). From what I can guess, Channels/Factories belong to FaceManager, which intentionally is destructed after Forwarder class (as it has to be created prior). Unless face is already "failed" while Forwarder instance is still alive, destruction of face inside Channel/Factory will result in signalling of failure to the forwarder, while the forwarder is already destructed. I guess, this specific part can be fixed by saving connections and disconnecting them inside FaceTable destructor.

Junxiao Shi wrote:

Why can't Android app destroy and recreate io_service?

I can. The bug is not exactly about ability to "cheat" (and I'm already doing it). The bug is about correctness during the destruction process.

Alex Afanasyev wrote:

Unless face is already "failed" while Forwarder instance is still alive, destruction of face inside Channel/Factory will result in signalling of failure to the forwarder, while the forwarder is already destructed. I guess, this specific part can be fixed by saving connections and disconnecting them inside FaceTable destructor.

Yes I guess FaceTable should use a ScopedConnection for its subscriptions to the faces then.

I rebased https://gerrit.named-data.net/1759 to patchset11, but I couldn't get the test case to work.

One problem is: FaceTable destructor expects Face::close (formerly Face::fail) to close the face immediately, but Face::close only requests the face to be closed, the actual closing is asynchronous.
Thus, FaceTable destructor is not the right place to request closing faces.

The caller needs to request the closure (possible through a helper method on FaceTable class), and then waits until all faces are closed before deallocating FaceTable.

Junxiao Shi wrote:

One problem is: FaceTable destructor expects Face::close (formerly Face::fail) to close the face immediately, but Face::close only requests the face to be closed, the actual closing is asynchronous.

Correct.

Thus, FaceTable destructor is not the right place to request closing faces.

This is only one possible conclusion. There is at least another one: FaceTable destructor should not be expecting all faces to be closed at the end of its execution. At that point the faces can be either CLOSING, FAILED, or CLOSED. The io_service should naturally return from run() once there are no more handlers to execute. After that, all faces will be CLOSED (in fact, they will be deallocated immediately after their last handler has returned).

This also means that we need a stop()/reset() method in Nfd or NfdRunner that triggers the destruction of everything.

The caller needs to request the closure (possible through a helper method on FaceTable class), and then waits until all faces are closed before deallocating FaceTable.

The problem here is that "the caller" is another destructor, so how do you "wait"? I don't know if it's a good idea to call io_service::poll() from a destructor.