PROTON-1442: [Cpp] Support for local transactions #437

DreamPearl · 2024-11-20T08:57:17Z

PROTON-1442

AMQP Transaction Sequence:

Declare transaction:

Client establishes link to Broker (Transaction resource) to target with transaction coordinator type (usual types are sender/receiver) (ATTACH frame)
Client (Transaction Controller) sends a special message to that link to create transaction (TRANSFER frame)
Broker returns a disposition with the transaction id (DISPOSITION frame)

Send Message in the transaction:

Client sends a message to the broker with transaction id in the state field.

Commit/Abort

astitcher

I've taken a fairly thorough look. If you have any questions about these review items we should discuss further.

cpp/include/proton/container.hpp

cpp/include/proton/target_options.hpp

cpp/include/proton/tracker.hpp

cpp/include/proton/transaction.hpp

astitcher · 2024-12-04T20:37:59Z

cpp/include/proton/transaction.hpp

+class
+PN_CPP_CLASS_EXTERN transaction_handler {
+  public:
+    PN_CPP_EXTERN virtual ~transaction_handler();
+
+    /// Called when a local transaction is declared.
+    PN_CPP_EXTERN virtual void on_transaction_declared(transaction);
+
+    /// Called when a local transaction is discharged successfully.
+    PN_CPP_EXTERN virtual void on_transaction_committed(transaction);
+
+    /// Called when a local transaction is discharged unsuccessfully (aborted).
+    PN_CPP_EXTERN virtual void on_transaction_aborted(transaction);
+
+    /// Called when a local transaction declare fails.
+    PN_CPP_EXTERN virtual void on_transaction_declare_failed(transaction);
+
+    /// Called when the commit of a local transaction fails.
+    PN_CPP_EXTERN virtual void on_transaction_commit_failed(transaction);
+};
+
+} // namespace proton
+


I think this should be in a different header file like message and messaging_handler

So if the API makes transactions hidden in a session then these callbacks either go away or instead become session callbacks: on_session_transaction_committed(session&), etc. I think transaction_declared() goes away entirely and it's purpose is now another use for on_session_open(session&). on_.._declare_failed should be handled by the on_session_error(session&).

Updated. Now they are session callbacks, transaction_declared() becomes handler->on_session.open() and on_declare_failed is now handled by on_session_error().

cpp/include/proton/transaction.hpp

cpp/examples/tx_recv.cpp

astitcher

I've not reviewed this comprehensively, but I think I'm now very much leaning towards not making the transaction class visible to the API user at all and having the transaction methods on the session. This is essentially the API in JMS and CMS.

cpp/include/proton/transaction.hpp

astitcher · 2024-12-18T23:10:52Z

cpp/include/proton/transaction.hpp

+class
+PN_CPP_CLASS_EXTERN transaction_handler {
+  public:
+    PN_CPP_EXTERN virtual ~transaction_handler();
+
+    /// Called when a local transaction is declared.
+    PN_CPP_EXTERN virtual void on_transaction_declared(transaction);
+
+    /// Called when a local transaction is discharged successfully.
+    PN_CPP_EXTERN virtual void on_transaction_committed(transaction);
+
+    /// Called when a local transaction is discharged unsuccessfully (aborted).
+    PN_CPP_EXTERN virtual void on_transaction_aborted(transaction);
+
+    /// Called when a local transaction declare fails.
+    PN_CPP_EXTERN virtual void on_transaction_declare_failed(transaction);
+
+    /// Called when the commit of a local transaction fails.
+    PN_CPP_EXTERN virtual void on_transaction_commit_failed(transaction);
+};
+
+} // namespace proton
+


So if the API makes transactions hidden in a session then these callbacks either go away or instead become session callbacks: on_session_transaction_committed(session&), etc. I think transaction_declared() goes away entirely and it's purpose is now another use for on_session_open(session&). on_.._declare_failed should be handled by the on_session_error(session&).

cpp/include/proton/transfer.hpp

cpp/examples/tx_send.cpp

pematous · 2025-01-21T15:35:45Z

cpp/examples/tx_recv.cpp

+        transaction.accept(d);
+        current_batch += 1;
+        if(current_batch == batch_size) {
+            transaction = proton::transaction(); // null


I believe we should do rather commit here, that way it works as expected and the receiver is closed after expected number of messages received (with the current implementation it's not).

I believe we should do rather commit here, that way it works as expected and the receiver is closed after expected number of messages received (with the current implementation it's not).

above mentioned change:

- transaction = proton::transaction(); // null + transaction.commit();

works as expected (against Artemis broker) when using multicast. However, when a pre-defined anycast queue is used, the commit() call ends with client segmentation fault.

cpp/examples/tx_recv.cpp

cpp/examples/tx_send.cpp

pematous · 2025-01-24T10:16:46Z

cpp/examples/tx_recv.cpp

+
+    void on_message(proton::delivery &d, proton::message &msg) override {
+        std::cout<<"# MESSAGE: " << msg.id() <<": "  << msg.body() << std::endl;
+        transaction.accept(d);


btw. I'm thinking if transaction.accept() does actually have any effect? I mean, when I comment it out, the program behaves the same. btw. is this transaction accept any different from delivery accept? looks both settles the messages related to the delivery.

Does explicit delivery settling play role in transaction mode? or are they mutually exclusive and only commits and aborts applies? Can ie. single message in transaction be rejected while other messages accepted?

I tried to accept / reject the delivery and it seems to have no effect in transaction mode (while in python, doing so makes the messages to be threat outside of the transaction, just like a normal messages, https://issues.redhat.com/browse/ENTMQCL-513).

cpp/src/messaging_adapter.cpp

astitcher

Lopts of detailed comments here, but the main change I'd like to see is either make coordinator a subclass of target or fold it into a simple boolean option of target.

cpp/examples/tx_recv.cpp

cpp/examples/tx_send.cpp

cpp/src/proton_bits.hpp

cpp/src/sender_options.cpp

cpp/src/transaction.cpp

cpp/include/proton/session.hpp

astitcher · 2025-04-24T13:51:24Z

cpp/include/proton/session.hpp

 struct pn_session_t;

 namespace proton {
+  class transaction_impl;


Is this actually needed here?

transaction_impl is pirely internal and should not be in an externally visible header file

cpp/include/proton/session.hpp

cpp/include/proton/target_options.hpp

cpp/include/proton/session.hpp

cpp/src/proactor_container_impl.cpp

cpp/examples/tx_send.cpp

cpp/src/session.cpp

astitcher

First batch of comments

cpp/examples/tx_recv.cpp

astitcher · 2025-08-28T16:08:10Z

cpp/include/proton/session.hpp

 struct pn_session_t;

 namespace proton {
+  class transaction_impl;


transaction_impl is pirely internal and should not be in an externally visible header file

cpp/include/proton/session.hpp

DreamPearl

Rebased!

cpp/src/messaging_adapter.cpp

DreamPearl · 2025-10-22T17:04:54Z

cpp/include/proton/transaction.hpp

+class
+PN_CPP_CLASS_EXTERN transaction_handler {
+  public:
+    PN_CPP_EXTERN virtual ~transaction_handler();
+
+    /// Called when a local transaction is declared.
+    PN_CPP_EXTERN virtual void on_transaction_declared(transaction);
+
+    /// Called when a local transaction is discharged successfully.
+    PN_CPP_EXTERN virtual void on_transaction_committed(transaction);
+
+    /// Called when a local transaction is discharged unsuccessfully (aborted).
+    PN_CPP_EXTERN virtual void on_transaction_aborted(transaction);
+
+    /// Called when a local transaction declare fails.
+    PN_CPP_EXTERN virtual void on_transaction_declare_failed(transaction);
+
+    /// Called when the commit of a local transaction fails.
+    PN_CPP_EXTERN virtual void on_transaction_commit_failed(transaction);
+};
+
+} // namespace proton
+


Updated. Now they are session callbacks, transaction_declared() becomes handler->on_session.open() and on_declare_failed is now handled by on_session_error().

DreamPearl · 2025-10-22T17:05:35Z

cpp/include/proton/transaction.hpp

+    PN_CPP_EXTERN void declare();
+    PN_CPP_EXTERN void handle_outcome(proton::tracker);
+    PN_CPP_EXTERN proton::tracker send(proton::sender s, proton::message msg);
+    PN_CPP_EXTERN void accept(delivery &t);


DreamPearl · 2025-10-22T17:07:48Z

cpp/include/proton/session.hpp

 struct pn_session_t;

 namespace proton {
+  class transaction_impl;


astitcher · 2025-11-10T15:44:26Z

Running the transaction test under valgrind reveals some errors: Transaction Test Errors

astitcher

I've made a bunch of suggested changes to this branch which address some of the comments here

astitcher · 2025-11-10T15:52:03Z

cpp/examples/simple_recv.cpp

None of the changes in this file relate to transactions, I recommend removing all changes in this file:
The change is useful, but something unnecessary for a simple example, moving the code more in the direction of a generally useful utility. Having such a general receive utility would be good, but maybe should be in a different place.

astitcher · 2025-11-11T17:27:13Z

cpp/include/proton/session.hpp

+    PN_CPP_EXTERN void transaction_declare(proton::messaging_handler &handler, bool settle_before_discharge = false);
+    PN_CPP_EXTERN bool transaction_is_declared();
+    PN_CPP_EXTERN proton::binary transaction_id() const;
+    PN_CPP_EXTERN void transaction_commit();
+    PN_CPP_EXTERN void transaction_abort();


Need documentation for these APIs

astitcher · 2025-11-11T17:54:50Z

cpp/include/proton/target_options.hpp

+    /// Set the target be of type coordinator.
+    /// This immediately override the currently assigned type.
+    PN_CPP_EXTERN target_options& make_coordinator();
+


I don't think this is needed to use the transaction API. If it's used in the implementation it needs to be purely internal. But I don't think it's actually used at all.

astitcher · 2025-11-11T17:58:31Z

cpp/src/contexts.hpp

 #include "reconnect_options_impl.hpp"

 #include "proton/work_queue.hpp"
+#include "proton/session.hpp"


I can't see a reason this include should be needed by the new lines.

astitcher · 2025-11-11T18:08:19Z

cpp/src/messaging_adapter.cpp


 #include <proton/connection.h>
 #include <proton/delivery.h>
+#include <proton/disposition.h>


Why is this needed?

astitcher · 2025-11-13T13:40:15Z

cpp/src/session.cpp

+            state = State::FREE;
+            transaction_delete(session);
+            // on_transaction_declare_failed
+            handler->on_session_error(session);


Use after free: handler is used after the transaction_impl is deleted! Also no need to set the state as the impl is just about to be deleted anyway.

astitcher · 2025-11-13T13:40:45Z

cpp/src/session.cpp

+                state = State::FREE;
+                transaction_delete(session);
+                handler->on_session_transaction_commit_failed(session);


Use after free again

astitcher · 2025-11-13T13:41:15Z

cpp/src/session.cpp

+                state = State::FREE;
+                transaction_delete(session);


No need for state change as deleted

astitcher · 2025-11-13T13:41:35Z

cpp/src/session.cpp

+                state = State::FREE;
+                transaction_delete(session);
+                handler->on_session_transaction_aborted(session);


Use after free

astitcher · 2025-11-13T13:41:57Z

cpp/src/session.cpp

+                state = State::FREE;
+                transaction_delete(session);
+                handler->on_session_transaction_committed(session);


Use after free

astitcher · 2025-11-13T23:13:35Z

My suggested changes are in branch https://github.com/astitcher/qpid-proton/commits/local-transactions
The last commit in that branch is the relevant commit.
I will carry on working on that tomorrow to see how far I can progress it before the weekend.

DreamPearl marked this pull request as draft November 20, 2024 08:57

DreamPearl force-pushed the local-transactions branch 2 times, most recently from 74818fc to eb4514b Compare November 21, 2024 18:28

DreamPearl force-pushed the local-transactions branch from 3e96834 to fa9236d Compare December 3, 2024 12:56

astitcher requested changes Dec 4, 2024

View reviewed changes

DreamPearl force-pushed the local-transactions branch from fa9236d to 347cf6e Compare December 16, 2024 13:29

astitcher requested changes Dec 18, 2024

View reviewed changes

pematous reviewed Jan 21, 2025

View reviewed changes

cpp/examples/tx_send.cpp Outdated Show resolved Hide resolved

pematous reviewed Jan 21, 2025

View reviewed changes

cpp/examples/tx_recv.cpp Outdated Show resolved Hide resolved

pematous reviewed Jan 24, 2025

View reviewed changes

cpp/examples/tx_send.cpp Show resolved Hide resolved

pematous reviewed Jan 24, 2025

View reviewed changes

cpp/examples/tx_send.cpp Outdated Show resolved Hide resolved

pematous reviewed Jan 24, 2025

View reviewed changes

DreamPearl force-pushed the local-transactions branch from 4c795ef to eb8afbd Compare March 17, 2025 17:54

pematous reviewed Mar 18, 2025

View reviewed changes

cpp/src/messaging_adapter.cpp Outdated Show resolved Hide resolved

astitcher requested changes Mar 20, 2025

View reviewed changes

DreamPearl force-pushed the local-transactions branch from eb8afbd to 4c795ef Compare March 26, 2025 05:04

DreamPearl force-pushed the local-transactions branch from 578445f to 6fd1434 Compare April 17, 2025 12:39

astitcher reviewed Apr 24, 2025

View reviewed changes

astitcher reviewed Aug 28, 2025

View reviewed changes

DreamPearl force-pushed the local-transactions branch 3 times, most recently from 6eb58f4 to 2d440ef Compare October 4, 2025 05:20

DreamPearl force-pushed the local-transactions branch 2 times, most recently from 3dcb6a7 to f04bc43 Compare October 30, 2025 13:14

DreamPearl added 5 commits November 2, 2025 17:51

Implement local transaction in CPP

4e40be5

Local Transaction tx_recv.cpp example shows abort() function

41419db

Update tx_send and tx_recv examples to fully work

cb6b064

Add a test file and few improvements

63d16bb

Rebase and fix error 'no data'

ac1950f

DreamPearl force-pushed the local-transactions branch from f04bc43 to ac1950f Compare November 3, 2025 06:18

DreamPearl commented Nov 3, 2025

View reviewed changes

DreamPearl marked this pull request as ready for review November 3, 2025 08:54

Fix valgrind errors

38bd553

astitcher requested changes Nov 13, 2025

View reviewed changes

astitcher self-assigned this Nov 13, 2025

PROTON-1442: [Cpp] Support for local transactions #437

Are you sure you want to change the base?

PROTON-1442: [Cpp] Support for local transactions #437

Uh oh!

Conversation

DreamPearl commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astitcher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

astitcher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pematous Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

astitcher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

astitcher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DreamPearl commented Nov 20, 2024 •

edited

Loading

pematous Feb 5, 2025 •

edited

Loading