Project

General

Profile

Task #2300

Face: use Signal

Added by Junxiao Shi almost 5 years ago. Updated over 4 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Faces
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
2.00 h

Description

Change Face API to use ndn::util::signal::Signal in on(Receive|Send)(Interest|Data).


Related issues

Blocks ndn-cxx - Bug #2213: EventEmitter::clear() leads to double free when called from Face::fail()Rejected

Blocks NFD - Feature #1999: Strategy for access routerClosed

Blocked by ndn-cxx - Bug #2329: Signal: emitSignal doesn't compile when used with non-zero-argument SignalClosed2014-12-25

Blocked by ndn-cxx - Feature #2331: Signal: onceClosed

History

#1 Updated by Junxiao Shi almost 5 years ago

  • Blocks Bug #2213: EventEmitter::clear() leads to double free when called from Face::fail() added

#2 Updated by Junxiao Shi almost 5 years ago

#3 Updated by Junxiao Shi over 4 years ago

  • Status changed from New to In Progress
  • Assignee set to Junxiao Shi

#4 Updated by Junxiao Shi over 4 years ago

  • Blocked by Bug #2329: Signal: emitSignal doesn't compile when used with non-zero-argument Signal added

#5 Updated by Junxiao Shi over 4 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 80

http://gerrit.named-data.net/1570

Change won't compile until #2329 is merged.


I'm unsure about one question:

Previously, EventEmitter::clear() is called in FaceTable::remove. But there's no Signal::clear().

Currently, FaceTable::remove does not attempt to disconnect signal connections, and most connections are not saved and are not disconnected until Face itself is destructed.
This doesn't break any test case.

Could this cause other potential problems?

#6 Updated by Davide Pesavento over 4 years ago

It depends on when the face is destructed... unfortunately there is no guarantee on that, but it seems cleaner to me if the FaceTable removes its subscriptions when it's done with the face. And this covers onReceive(Interest|Data) and onFail.

As far as onSend(Interest|Data) are concerned, I don't know why they are cleared in the current code, maybe for the same reason as #1718?

#7 Updated by Junxiao Shi over 4 years ago

  • Status changed from Resolved to In Progress

note-6 pointed out the problem: shared_ptr to the face itself in bound functions would prevent the face from being destructed.

Thus, Connection for a handler which holds a shared_ptr to the face must be stored somewhere, and be disconnected upon onFail.

#8 Updated by Davide Pesavento over 4 years ago

On the other hand, wouldn't it be more appropriate to use a weak_ptr in those cases?

More generally, we should really define who owns the various face objects... FaceTable or the protocol factories? or someone else? Right now they aren't really owned by anyone... in fact they're collectively owned by anything that has a shared_ptr to them, which doesn't give many guarantees, and that makes it harder to reason about object lifetime, destruction and ownership cycles.

#9 Updated by Junxiao Shi over 4 years ago

#10 Updated by Junxiao Shi over 4 years ago

I somewhat agree with note-8, however that's out of scope of this Task.

The solution to #1718 regression is:

  • use weak_ptr or reference instead of shared_ptr in bound handlers
  • if shared_ptr is desirable, store the Connection, and cancel it in onFail
  • use connectSingleShot (#2331) for onFail handlers

#11 Updated by Junxiao Shi over 4 years ago

  • Status changed from In Progress to Code review
  • % Done changed from 80 to 100

I have implemented note-10 solution, and checked UdpFace has no #1718 regression.

#12 Updated by Junxiao Shi over 4 years ago

  • Status changed from Code review to Closed

Also available in: Atom PDF