Skip to content

Conversation

@ivanpauno
Copy link
Collaborator

After going a bit on circles, I think I have found an approach I'm happy with.

I don't like much the approach of passing a Class<T> and relying on runtime reflection. That's usually a java anti-pattern (except when you really need it).
Instead of passing a class, I'm passing an "event status factory" (that's what actually needed).

I'm also trying to avoid giving the user the possibility of shooting in their own feet using dispose
e.g.:

node.dispose();
publisher.dispose(); // the node was already disposed, the publisher will use an invalid node handle -> UB

That's kind of bad ....
I tried to solve the problem by ensuring that:

  • publisher.dispose() will also dispose it's child events.
  • doing event_handler.dispose() twice isn't a problem.

I guess that's a better pattern ...

There will still be a problem when disposing while the EventHandler is used by an Executor.
Handling that correctly will require a big refactor, which is not the goal here (I only tried to avoid repeating the pre-existing problems where it was easy to do so).

Next step:

  • Adding support to events in Executor.

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno ivanpauno added the enhancement New feature or request label Aug 14, 2020
@ivanpauno ivanpauno requested a review from jacobperron August 14, 2020 21:21
@ivanpauno ivanpauno self-assigned this Aug 14, 2020
@ivanpauno
Copy link
Collaborator Author

This is targeting feature/events branch, as the code isn't really usable yet (no executor support)

Copy link

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach looks good to me. Some minor comments and suggestions below.

@ivanpauno ivanpauno requested a review from jacobperron August 18, 2020 15:00
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno ivanpauno merged commit e229771 into feature/events Aug 18, 2020
@ivanpauno ivanpauno deleted the ivanpauno/publisher-liveliness-lost-event branch August 18, 2020 18:35
@ivanpauno
Copy link
Collaborator Author

Merged on the feature branch, thanks for the review @jacobperron.
Next step: support in the executor.

ivanpauno added a commit that referenced this pull request Aug 20, 2020
* Create LivelinessLostEvent

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Fix linking issue

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Rename disposeEventHandler to removeEventHandler

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Use synchronized methods in EventHandlerImpl

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Use createEventHandler instead of registerEventHanlder

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* nit

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Clarify ownership of the event handle

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
ivanpauno added a commit that referenced this pull request Aug 31, 2020
* Create LivelinessLostEvent

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Fix linking issue

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Rename disposeEventHandler to removeEventHandler

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Use synchronized methods in EventHandlerImpl

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Use createEventHandler instead of registerEventHanlder

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* nit

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Clarify ownership of the event handle

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
ivanpauno added a commit that referenced this pull request May 17, 2021
* Create LivelinessLostEvent

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Fix linking issue

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Rename disposeEventHandler to removeEventHandler

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Use synchronized methods in EventHandlerImpl

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Use createEventHandler instead of registerEventHanlder

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* nit

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Clarify ownership of the event handle

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
ivanpauno added a commit to ros2-java/ros2_java that referenced this pull request Dec 3, 2021
* Create LivelinessLostEvent

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Fix linking issue

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Rename disposeEventHandler to removeEventHandler

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Use synchronized methods in EventHandlerImpl

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Use createEventHandler instead of registerEventHanlder

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* nit

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Clarify ownership of the event handle

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
ivanpauno added a commit to ros2-java/ros2_java that referenced this pull request Dec 6, 2021
* Create LivelinessLostEvent

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Fix linking issue

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Rename disposeEventHandler to removeEventHandler

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Use synchronized methods in EventHandlerImpl

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Use createEventHandler instead of registerEventHanlder

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* nit

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Clarify ownership of the event handle

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
ivanpauno added a commit to ros2-java/ros2_java that referenced this pull request Dec 6, 2021
* Create LivelinessLostEvent

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Fix linking issue

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Rename disposeEventHandler to removeEventHandler

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Use synchronized methods in EventHandlerImpl

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Use createEventHandler instead of registerEventHanlder

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* nit

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Clarify ownership of the event handle

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants