New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot add more than one listener if its expressed as a lambda #95

Closed
jeffdiclemente opened this Issue May 11, 2016 · 2 comments

Comments

Projects
1 participant
@jeffdiclemente
Contributor

jeffdiclemente commented May 11, 2016

I noticed this while implementing the framework listeners; doing the following does not add the second listener:

int count1(0);
int count2(0);
auto listener_callback_counter1 = [&count1](const FrameworkEvent& evt) { ++count1; std::cout << "listener_callback_counter1: call count " << count1 << std::endl; };
auto listener_callback_counter2 = [&count2](const FrameworkEvent& evt) { ++count2; std::cout << "listener_callback_counter2: call count " << count2 << std::endl; };
f->GetBundleContext()->AddFrameworkListener(listener_callback_counter1);
f->GetBundleContext()->AddFrameworkListener(listener_callback_counter2);

The same happens when adding bundle listeners.
In both cases the listener comparison functions in usServicveListeners.cpp returns true for the second listener as compared to the first.

If the std::bind flavor of AddBundleListener is used, there is no problem.

For users, it is normal to express std::function objects as lambdas and expect this to work.

It is difficult to compare the equality of std::function objects as evidenced by this stackoverflow thread.
We'll have to define what equality really means for listeners.

@jeffdiclemente jeffdiclemente self-assigned this May 11, 2016

@jeffdiclemente jeffdiclemente added the bug label May 11, 2016

@jeffdiclemente

This comment has been minimized.

Show comment
Hide comment
@jeffdiclemente

jeffdiclemente May 18, 2016

Contributor

Some more info from further investigation...

Since std::function::target<void(const BundleEvent&)>() is used as one of the criteria to compare listeners for equality lambdas and callable objects created from std::bind will never be compared correctly because target<T>() will return a pointer only if the stored callable object is of type T, otherwise it returns nullptr. For lambdas and callable objects created from std::bind, the type is not void(const BundleEvent& ).

The only reason listeners created from std::bind can be added is because the pointer to the class instance is also used to compare the listeners. As long as two different class instance pointers are used the listeners aren't equal.

Contributor

jeffdiclemente commented May 18, 2016

Some more info from further investigation...

Since std::function::target<void(const BundleEvent&)>() is used as one of the criteria to compare listeners for equality lambdas and callable objects created from std::bind will never be compared correctly because target<T>() will return a pointer only if the stored callable object is of type T, otherwise it returns nullptr. For lambdas and callable objects created from std::bind, the type is not void(const BundleEvent& ).

The only reason listeners created from std::bind can be added is because the pointer to the class instance is also used to compare the listeners. As long as two different class instance pointers are used the listeners aren't equal.

ksubramz added a commit to ksubramz/CppMicroServices that referenced this issue Dec 28, 2016

Support multiple listeners
Proof of concept implementation of supporting multiple listeners. The multiple
listeners implementation was present but didn't work as evidenced in bugs CppMicroServices#83
and CppMicroServices#95.

This adds a templated AddFrameworkListener that takes callables with distinct
addresses - free functions and functors. To achieve this, we make use of the
compile time traits utility FunctionTraits.h

Also adds implementation of the FrameworkToken object and an internal atomic id
for listeners.

The FrameworkListenerTest is changed to support just the multiple listeners for
the time being.

ksubramz added a commit to ksubramz/CppMicroServices that referenced this issue Jan 3, 2017

Support multiple listeners
Proof of concept implementation of supporting multiple listeners. The multiple
listeners implementation was present but didn't work as evidenced in bugs CppMicroServices#83
and CppMicroServices#95.

This adds a templated AddFrameworkListener that takes callables with distinct
addresses - free functions and functors. To achieve this, we make use of the
compile time traits utility FunctionTraits.h

Also adds implementation of the FrameworkToken object and an internal atomic id
for listeners.

The FrameworkListenerTest is changed to support just the multiple listeners for
the time being.

@saschazelzer saschazelzer added this to Planned in Release 3.1 Feb 13, 2017

ksubramz added a commit to ksubramz/CppMicroServices that referenced this issue Feb 26, 2017

Support multiple listeners.
Fix issues CppMicroServices#83 and CppMicroServices#95.

Change the BundleContext add listeners functions to return ListenerToken
objects.

Add API function RemoveListeners which can use these
ListenerToken objects to remove the listeners corresponding to the
tokens. Removing listeners using their tokens is the recommended
approach to clients going forward.

Introduce the move-only ListenerToken type. This is a wrapper to
ListenerTokenId, an uint64_t type, which is incremented every-time
a listener of any type is added to the BundleContext.

Deprecate existing API functions RemoveFrameworkListener,
RemoveBundleListener and RemoveServiceListener. These will be
removed in the next major release. These functions also emit
diagnostic log messages that they are deprecated.

Change framework code to use tokens in-place of the deprecated
RemoveServiceListener calls.

Change ServiceListenerEntry to contain ListenerTokenId.

Enable the previously commented out multiple framework listeners
test block in FrameworkListenerTest.

Add a new test file named MultipleListenersTest which exercises
this new functionality in a variety of ways.

Signed-off-by: The Mathworks Inc <Roy.Lurie@mathworks.com>

@jeffdiclemente jeffdiclemente moved this from Planned to In Progress in Release 3.1 Mar 10, 2017

jeffdiclemente added a commit that referenced this issue Apr 12, 2017

Support multiple listeners. (#179)
* Support multiple listeners.

Fix issues #83 and #95.

Change the BundleContext add listeners functions to return ListenerToken
objects.

Add API function RemoveListeners which can use these
ListenerToken objects to remove the listeners corresponding to the
tokens. Removing listeners using their tokens is the recommended
approach to clients going forward.

Introduce the move-only ListenerToken type. This is a wrapper to
ListenerTokenId, an uint64_t type, which is incremented every-time
a listener of any type is added to the BundleContext.

Deprecate existing API functions RemoveFrameworkListener,
RemoveBundleListener and RemoveServiceListener. These will be
removed in the next major release. These functions also emit
diagnostic log messages that they are deprecated.

Change framework code to use tokens in-place of the deprecated
RemoveServiceListener calls.

Change ServiceListenerEntry to contain ListenerTokenId.

Enable the previously commented out multiple framework listeners
test block in FrameworkListenerTest.

Add a new test file named MultipleListenersTest which exercises
this new functionality in a variety of ways.

Signed-off-by: The Mathworks Inc <Roy.Lurie@mathworks.com>

* Support multiple listeners.

Fix issues in Mac. Temporarily removed US_DEPRECATED because it resulted in
build failures.

* Support multiple listeners.

Fix based on Sascha's PR review.

Modify bundle and framework listeners to be an unordered_map with
ListenerTokenIds as keys. This improves the removal of these types of listeners
to O(1).

Deprecate addition of listeners functions which take member functions of
objects. Clients can use std::bind together with the other Add*Listener
functions to achieve the same result.

Modify RemoveListener call to take token by value. With this change, clients
have to explicitly move the tokens.

* Support multiple listeners.

Change the container holding futures to array, to remove Run time check error #2
reported by VS2013. The issue is pending investigation.

* Support multiple listeners.

Incorporate suggestions by Jeff and Abhinay after code review.

Clean up comments.
Introduce mechanisms in the test code to remove warnings due to deprecated
functions. De-deprecate Add.*Listener functions which take member
functions.
Reduce code duplication by cleaning up unneeded constructors.
Improve concurrency test of addition multiple listeners.

* Support multiple listeners.

Fix because of Mac build failures. Minor improvements.

* Support multiple listeners.

Fix GCC 4.9.2 build failures. Move the disabling of deprecation warning up to
CMake level and add means to disable the deprecation warning per source file.

Deprecate the Add.*Listener functions that take the member function pointers.
Modify the instances where service listeners are added to take the std::bind
objects instead.

* Support multiple listeners.

Remove a stray trailing macro.

* Support multiple listeners.

Fix a sporadic threading related crash by usage of Framework::WaitForStop().

Replace ordered_map::emplace use with [] because emplace isn't supported by
older GCC compilers.

* Support mutliple listeners.

Change based on Sascha's review.

* Support multiple listeners.

Add a test that adds and removes listeners in a true concurrent fashion.

Change based on Sascha and Jeff's comments.

* Fix Mac build failures.

* Support multiple listeners.

Added a workaround for a Visual Studio 2013 compiler bug.

* Support multiple listeners.

Factor out ListenerToken into a separate header and implementation file.

* Support multiple listeners.

Add missing file

* Support multiple changes.

Fix based on review

* Support multiple listeners.

Improve minor things - remove unnecessary constructor and fix API doc.

* Support multiple listeners.

Templatize testConcurrentAddRemove function to work with all 3 types of
listeners.

* Support multiple listeners.

Fix Clang bug in Mac.

* Support multiple listeners.

Fix to build on GCC 4.6

* Support multiple listeners.

Change based on Jeff's review.

* Support multiple listeners.

Change based on review (minor).

Signed-off-by: The Mathworks Inc Roy.Lurie@mathworks.com
@jeffdiclemente

This comment has been minimized.

Show comment
Hide comment
@jeffdiclemente

jeffdiclemente Apr 12, 2017

Contributor

fixed in PR #179

Contributor

jeffdiclemente commented Apr 12, 2017

fixed in PR #179

@ksubramz ksubramz moved this from In Progress to Done in Release 3.1 Apr 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment