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

Removing Listeners does not work well #83

Closed
karthikreddy09 opened this Issue Mar 1, 2016 · 2 comments

Comments

Projects
2 participants
@karthikreddy09
Contributor

karthikreddy09 commented Mar 1, 2016

we use std::function objects for listeners.

  1. The listener has to be a function pointer. Anything else used does not work. e.g: lamdas
  2. When the same service listener is added with different filters, the remove is ambigious

@jeffdiclemente jeffdiclemente self-assigned this Jul 22, 2016

@jeffdiclemente jeffdiclemente added the bug label Jul 22, 2016

@jeffdiclemente

This comment has been minimized.

Show comment
Hide comment
@jeffdiclemente

jeffdiclemente Jul 22, 2016

Contributor

This is related to #95 as in solving one can also solve the other.

Contributor

jeffdiclemente commented Jul 22, 2016

This is related to #95 as in solving one can also solve the other.

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.

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 added this to the Release 3.1 milestone Mar 10, 2017

@jeffdiclemente jeffdiclemente added this 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