Project

General

Profile

Actions
Copy link

Feature #2279

closed

Signal: allow only owner to emit events

Added by Alex Afanasyev over 9 years ago. Updated over 9 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Junxiao Shi
Category:
Utils
Target version:
v0.3
Start date:
Due date:
% Done:

100%

Estimated time:
3.00 h

Description

template<typename Owner, typename ...TArgs>
class Signal
{
...
private:
  void
  operator()(const TArgs&... args) const;

  friend Owner;
};

class Face
{
public:
  Signal<Face, Interest> onReceiveInterest;

protected:
  void
  emit_onReceiveInterest(const Interest& interest)
  {
    onReceiveInterest(interest);
  }
};

Related issues 3 (0 open — 3 closed)

Related to ndn-cxx - Task #2116: Rewrite EventEmitterClosedJunxiao Shi

Actions
Blocks NFD - Feature #2272: Strategy API: access to FaceTableClosedJunxiao Shi

Actions
Blocks ndn-cxx - Feature #1529: PIB serviceAbandonedYingdi Yu

Actions
Actions
Copy link
 #1

Updated by Alex Afanasyev over 9 years ago

  • Related to Task #2116: Rewrite EventEmitter added
Actions
Copy link
 #2

Updated by Junxiao Shi over 9 years ago

  • Tracker changed from Task to Feature
  • Subject changed from Proposal for protecting emission events by the EventEmitter to EventEmitter: allow only owner to emit events
  • Description updated (diff)
  • Category set to Utils
  • Assignee set to Junxiao Shi
  • Target version set to v0.3

I agree with this idea. Some details are changed to make it work.

But this is a backwards-incompatible change. I suggest:

  • abandon the pending change of #2116
  • keep EventEmitter as is but mark it deprecated
  • create a Signal template that takes Owner template argument
  • use Boost.Signals2 naming in Signal template
  • Signal<TArgs...>::operator+= is not declared
Actions
Copy link
 #3

Updated by Alex Afanasyev over 9 years ago

I agree with the proposed plan with the exception that we remove objective of event unsubscribing (mark them as strikethrough, not just delete. so it will be clear what all conversations are about) and declare the task closed.

Actions
Copy link
 #4

Updated by Alex Afanasyev over 9 years ago

Btw. Anybody knows if boost.signal2 or other signal libraries provide protection for event emission? If so, how do they do that?

Actions
Copy link
 #5

Updated by Junxiao Shi over 9 years ago

Boost.Signals2 doesn't have protection.

C# has protection, which is a built-in feature of Common Language Runtime.

Actions
Copy link
 #6

Updated by Davide Pesavento over 9 years ago

Alex Afanasyev wrote:

Btw. Anybody knows if boost.signal2 or other signal libraries provide protection for event emission? If so, how do they do that?

In Qt signals are protected member functions, so this is not a problem for them.

Actions
Copy link
 #7

Updated by Davide Pesavento over 9 years ago

There's one thing I don't like about the proposed solution: Owner has complete access to all EventEmitter internals, not just to signal emission.

Actions
Copy link
 #8

Updated by Junxiao Shi over 9 years ago

  • Subject changed from EventEmitter: allow only owner to emit events to Signal: allow only owner to emit events
  • Description updated (diff)
  • Status changed from New to In Progress
  • Estimated time set to 3.00 h
Actions
Copy link
 #9

Updated by Junxiao Shi over 9 years ago

  • Description updated (diff)
  • Status changed from In Progress to Code review
  • % Done changed from 0 to 100

Benchmark

// g++ x.cpp --std=c++0x -O3 `pkg-config --cflags --libs libndn-cxx`

#include <ndn-cxx/util/time.hpp>
#include <ndn-cxx/util/event-emitter.hpp>
#include <ndn-cxx/util/signal.hpp>

int g_sum;

void
f(int n)
{
  g_sum += n;
}

struct EventEmitterOwner
{
  ndn::util::EventEmitter<int> ee;

  void
  run()
  {
    for (int i = 0; i <= 99999999; ++i) {
      ee(1);
    }
  }
};

struct SignalOwner
{
  ndn::util::Signal<SignalOwner, int> sig;

  void
  run()
  {
    for (int i = 0; i <= 99999999; ++i) {
      sig(1);
    }
  }
};

int
main()
{
  g_sum = 0;
  EventEmitterOwner eo;
  eo.ee += &f;
  auto t1 = ndn::time::system_clock::now();
  eo.run();
  auto t2 = ndn::time::system_clock::now();
  std::cout << g_sum << " " << (t2 - t1) << std::endl;

  g_sum = 0;
  SignalOwner so;
  so.sig.connect(&f);
  auto t3 = ndn::time::system_clock::now();
  so.run();
  auto t4 = ndn::time::system_clock::now();
  std::cout << g_sum << " " << (t4 - t3) << std::endl;

  return 0;
}

Result for commit 6452abf877a50aa8503b7053b37331d782ad2b29:

OS EventEmitter Signal
Ubuntu 12.04 6.806ns/op 4.905ns/op
OSX 10.9 4.870ns/op 4.549ns/op
Actions
Copy link
 #10

Updated by Junxiao Shi over 9 years ago

Actions
Copy link
 #11

Updated by Davide Pesavento over 9 years ago

Given the complexity and ugliness of the resulting implementation (especially the part to make signal emission from subclasses work), and given the fact that Signal internals are completely exposed to the Owner class, I'm not sure if this is worthwhile anymore, not in its current form at least (but I don't have an alternative solution).

Actions
Copy link
 #12

Updated by Alex Afanasyev over 9 years ago

I had similar comment, but haven't mentioned it before... Also not sure if it worth the effort to do such complex thing that results in not so nice syntax... We could give a try for the latest version, but I'm having doubts that we will not refert that in the future.

Actions
Copy link
 #13

Updated by Junxiao Shi over 9 years ago

  • Status changed from Code review to Closed
Actions
Copy link
 #14

Updated by Junxiao Shi about 9 years ago

Actions
Copy link

Also available in: Atom PDF