astitcher commented on a change in pull request #309:
URL: https://github.com/apache/qpid-proton/pull/309#discussion_r616843329
##########
File path: cpp/src/sender.cpp
##########
@@ -64,10 +64,18 @@ namespace {
uint64_t tag_counter = 0;
Review comment:
tag_counter is effectively a global and not used in a thread safe way.
This is the libraries responsibility not the user application. So there is a
clear threading bug here in the sender logic.
The underlying C proton library is "thread neutral" as long as you process
whole connections in a thread serialised way. However this piece of the C++
library is clearly at best racy giving duplicate ids out to different threads
and at worst a bug risk. We can fix it by using C++ atomics now that we can use
C++11.
##########
File path: cpp/include/proton/sender.hpp
##########
@@ -55,7 +55,7 @@ PN_CPP_CLASS_EXTERN sender : public link {
PN_CPP_EXTERN void open(const sender_options &opts);
/// Send a message on the sender.
- PN_CPP_EXTERN tracker send(const message &m);
+ PN_CPP_EXTERN tracker send(const message &m, const binary &tag = binary());
Review comment:
It would actually be better to add a new signature for
```sender::send()```: This is because of considerations of ABI compatibility.
When you change the signature by adding new parameters to a function you
change the 'mangled' symbol name that the compiler uses to ensure that programs
get the correct overload of a function when linking the program together. This
means that any existing compiled program (that is not recompiled) will not find
the symbol for ```sender::send()``` that it was originally linked with.
So in this case to ensure that existing programs continue to work and not to
cause an ABI breakage it would be better to introduce a new ```tracker
send(const message &m, const binary &tag)``` overload. Note that this needs to
be without the defaulted tag parameter for this to work. In the actual
implementations you can have one implementation call the other to avoid
cut-n-paste programming!
##########
File path: cpp/src/sender.cpp
##########
@@ -64,10 +64,18 @@ namespace {
uint64_t tag_counter = 0;
}
-tracker sender::send(const message &message) {
- uint64_t id = ++tag_counter;
- pn_delivery_t *dlv =
- pn_delivery(pn_object(), pn_dtag(reinterpret_cast<const char*>(&id),
sizeof(id)));
+tracker sender::send(const message &message, const binary &tag) {
Review comment:
As above, I think you should keep the original ```sender::send(const
message&)``. There are a few ways to make sure that both functions share most
of the code rather than cutting and pasting the common parts - you can decide
how to do it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
