Skip to content
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

Port of C++ binding to Proton Proactor #105

Merged
merged 22 commits into from
Jul 21, 2017
Merged

Conversation

astitcher
Copy link
Member

This set of changes reimplements the C++ binding do that it no longer uses the Proton Reactor library at all and now only links to the Proton Core and Proton Proactor libraries.

It is not 100% finished yet, missing pieces include:

  • Multi thread support - Currently the API is thread safe if you obey the thread safety rules, but only one thread is used to service handler callbacks - This will be fixed soon.
  • Reconnect is not implemented - If a network connection fails no reconnect action is attempted - This will also be fixed, but ntoe that the C++ binding implementation of reconnect is subject to change as we don't have a lot of experience with it yet.

Included in this PR is a reworking of the mechanism for injecting work into safe contexts. The concepts have been renamed for clarity (proton::work_queue & proton::work) and some convenience functions have been added to help those (poor benighted souls) using C++03. There is some more work to come which should also improve injecting work using the scheduler.

@@ -329,11 +329,7 @@ class message {
mutable annotation_map message_annotations_;
mutable annotation_map delivery_annotations_;

/// Decode the message corresponding to a delivery from a link.
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this relate to the other changes here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a private function that didn't need exposing. So I moved it to the internals.

I can't remember anymore if this change was enabled by other things changed or not. But the code moved into message_adapter.cpp which also changed a lot in the same change.

@@ -0,0 +1,368 @@
#ifndef PROTON_EVENT_LOOP_HPP
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs a rename.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will fix.

@astitcher
Copy link
Member Author

I've now rebased and slightly modified some the previous commits.

Substantively I've addressed the work_queue header guard issue; finished the work dealing with scheduling work to occur in different contexts; renamed proton::defer to proton::schedule_work.

Note that the commits in the branch are not in the order display in this PR because they have been heavily reworked and reordered in rebases. So the creation time does not correspond to the commit order.

Copy link
Contributor

@alanconway alanconway left a comment

Choose a reason for hiding this comment

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

Looks like a good cleanup. I looked at your demos on the latest branch also, nits:

  • broker.cpp should mention in early comments that it is thread-safe. You've done an admirable job of making the code simple but the use of work_queue is a bit surprising unless you know this is intended for MT use.

  • camelCase? Did you have some sort of flash-back?

Very nice job - the demo convinces me even more strongly that we should eliminate thread_safe and make work_queue the centre of the threading story - augmenting it if needed.

void messaging_handler::on_sender_open(sender &) {}
void messaging_handler::on_sender_open(sender &l) {
if (l.uninitialized()) {
l.open(l.container().sender_options());
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it will throw if the listener is not associated with a container. I want to still support pure connection_driver based application that might not have a "real" container impl - we could have a "null container" with only a container ID. That can be fixed later - not an objection to this commit.

@astitcher astitcher force-pushed the cpp-proactor branch 3 times, most recently from dbe4815 to 42828c3 Compare May 30, 2017 08:15
@astitcher
Copy link
Member Author

I've now updated this with what I intend to be the final rebase before merging.

Everything is functionally there except for reconnect, which will be addressed in the 0.19 timescale.

  • Multhtreaded container is now implemented
  • The (still) experimental connection_driver now works.

@alanconway
Copy link
Contributor

I'm still happy with this. I have some ideas about simplifying the returned<>/thread_safe<> story but lets get this up on master as a starting point.

- Remove all reactor use
- Rearrange object context code
- Change container includes to proactor container includes
- Add sender/receiver options API to connection so we never need container in handlers
- Rework connection_driver remove all use of container
- Change signature of listener_handler callbacks to supply the listener
- Removed old low level proton event handling completely
- Now directly dispatch to the messaging_handler
- Moved private message::decode directly into message handling code
…the previous st broker

- The st broker didn't correctly respect the object access constraints from within handlers
- If not overridden then you get automatic outgoing matching xx_open
- If overridden then it is assumed you will handle the outgoing open
  yourself.
…rk type

- The work type can be created from std::function<void()> or void_function0
- and so pushes those c++11/C++03 differences into a single place
… to the work_queue header

Add efficient C++11 versions of work factories
Reorder defer arguments to be like std::bind
Add extra overloads for defer like std::bind (for free functions arbitrary work_queues)
- Propagate the result of work_queue::add
- Fix memory leak of proton::defer if the deferred function can't be called
- Add make_work convenience in place of std::bind
- Simplfy defer oveload options - have to specify work_queue
@asfgit asfgit merged commit b17671e into apache:master Jul 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants