Project

General

Profile

Feature #2279

Signal: allow only owner to emit events

Added by Alex Afanasyev almost 5 years ago. Updated almost 5 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Utils
Target version:
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

Related to ndn-cxx - Task #2116: Rewrite EventEmitterClosed

Blocks NFD - Feature #2272: Strategy API: access to FaceTableClosed

Blocks ndn-cxx - Feature #1529: PIB serviceAbandoned

History

#1 Updated by Alex Afanasyev almost 5 years ago

  • Related to Task #2116: Rewrite EventEmitter added

#2 Updated by Junxiao Shi almost 5 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

#3 Updated by Alex Afanasyev almost 5 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.

#4 Updated by Alex Afanasyev almost 5 years ago

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

#5 Updated by Junxiao Shi almost 5 years ago

Boost.Signals2 doesn't have protection.

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

#6 Updated by Davide Pesavento almost 5 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.

#7 Updated by Davide Pesavento almost 5 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.

#8 Updated by Junxiao Shi almost 5 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

#9 Updated by Junxiao Shi almost 5 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

#10 Updated by Junxiao Shi almost 5 years ago

#11 Updated by Davide Pesavento almost 5 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).

#12 Updated by Alex Afanasyev almost 5 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.

#13 Updated by Junxiao Shi almost 5 years ago

  • Status changed from Code review to Closed

#14 Updated by Junxiao Shi over 4 years ago

Also available in: Atom PDF