gemmellr commented on a change in pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#discussion_r773070502
##########
File path: cpp/src/link_test.cpp
##########
@@ -23,9 +23,114 @@
#include <proton/sender_options.hpp>
#include <proton/receiver_options.hpp>
#include <proton/container.hpp>
+#include <proton/connection.hpp>
+#include <proton/connection_options.hpp>
+#include <proton/listen_handler.hpp>
+#include <proton/listener.hpp>
+#include <proton/messaging_handler.hpp>
+#include <proton/types.hpp>
+#include <proton/message.hpp>
+#include <proton/target_options.hpp>
+#include <proton/source_options.hpp>
+#include <proton/delivery.hpp>
#include <iostream>
+#include <map>
+#include <condition_variable>
+#include <mutex>
+#include <thread>
+
+namespace {
+std::mutex m;
+std::condition_variable cv;
+bool listener_ready = false;
+int listener_port;
+} // namespace
+
+class test_recv : public proton::messaging_handler {
+ private:
+ class listener_ready_handler : public proton::listen_handler {
+ void on_open(proton::listener &l) override {
+ {
+ std::lock_guard<std::mutex> lk(m);
+ listener_port = l.port();
+ listener_ready = true;
+ }
+ cv.notify_one();
+ }
+ };
+
+ std::string url;
+ proton::listener listener;
+ listener_ready_handler listen_handler;
+
+ public:
+ test_recv(const std::string &s) : url(s) {}
+
+ void on_container_start(proton::container &c) override {
+ listener = c.listen(url, listen_handler);
+ }
+
+ void on_message(proton::delivery &d, proton::message &msg) override {
+ proton::symbol sym = "symbol";
+ proton::value val = "value";
+ std::map<proton::symbol, proton::value> props =
d.receiver().target().dynamic_node_properties();
+
+ ASSERT(!props.empty());
+ for(auto it=props.begin(); it!=props.end(); it++) {
+ ASSERT_EQUAL(sym, it->first);
+ ASSERT_EQUAL(val, it->second);
+ }
+ d.receiver().close();
+ d.connection().close();
+ listener.stop();
+ }
+};
+
+class test_send : public proton::messaging_handler {
+ private:
+ std::string url;
+ proton::sender sender;
+
+ public:
+ test_send(const std::string &s) : url(s) {}
+
+ void on_container_start(proton::container &c) override {
+ proton::target_options opts;
+ std::map<proton::symbol,proton::value> m({{proton::symbol("symbol"),
proton::value("value")}});
+ opts.dynamic_node_properties(m);
+ sender = c.open_sender(url, proton::sender_options().target(opts));
+ }
+
+ void on_sendable(proton::sender &s) override {
+ proton::message msg;
+ msg.body("message");
+ proton::tracker t = s.send(msg);
+ s.connection().close();
+ }
+};
+
+int test_dynamic_node_properties() {
Review comment:
That bit sounds/looks good. There are peripheral issues as mentioned
before that should also be addressed.
The trace log shows the test still uses an illegal target definition in the
clients sender attach, where it sets dynamic-node-properties, but doesnt set
dynamic=true, and sets an address "test" for the target (which the server
naively copies and so reflects back). Thats then compounded when the
test/client expects a 'response' attach with dynamic=true, something which
shouldn't occur in this scenario.
Right now it verifies there are [different] dynamic node properties on the
the servers 'response' attach, and the dynamic flag is set on the target, but
not that there is a generated address. It would be good for the test to verify
the whole of the 'dynamic' interation, i.e that the client 'requests'
dynamic=true and doesnt set an address, and that the client can pick up the
address value set by the server once the link opens (since the dynamic node
would be useless without an address).
I think it would be better if the server did its work in the
'on_receiver_open' (or per next comment, sender open) method rather than using
the options in the container init, since you wont/cant generally use the
options in that situation as a server as not everything is dynamic, and each
dynamic node needs its own address etc. When the link attaches the server could
verify it got a proper/expected request for a dynamic node, and respond setting
the appropriate details on the target directly (or probably rather the source,
per next comment) rather than using the options (EDIT: or it seems you can
supply link-specific options when calling open, perhaps that instead) and also
might as well open the link itself and not bothering with calling
messaging_handler::on_receiver_open(r);).
Main other comment would be that this is still only trying to test a
'dynamic sender' from the clients perspective, when almost noone will ever do
that, so it should either use a 'dynamic receiver' or test both.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
