Split poller_t into two layers #225

sigiesec · 2018-05-11T12:38:11Z

New poller_t (in zmq.hpp) is a direct mapping of the libzmq zmq_poller_* API.
New active_poller_t (in zmq_addon.hpp) is the previous poller_t, which is implemented based on the new poller_t, and provides a higher-level abstraction that actively manages callback handlers.

The PR contains some fixes to make zmq_addon.hpp compile again. It was broken before.

Solution: extract base_poller_t from poller_t, which provides a direct mapping of zmq_poller_* to C++ only

Solution: remove redundant assertions (tested elsewhere), and add assertion behaviour of calling wait on moved-from poller

Solution: convert to googletest based test, and fix syntax error

Solution: move to zmq_addon.hpp, rename to active_poller_t, and rename base_poller_t to poller_t

Solution: change to local variables

Solution: use non-deprecated operator!= instead

Solution: simplify code

Solution: add tests

Solution: removed redundant assertion

coveralls · 2018-05-11T12:41:55Z

Pull Request Test Coverage Report for Build 81

0 of 0 changed or added relevant lines in 0 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.7%) to 56.923%

Files with Coverage Reduction	New Missed Lines	%
zmq.hpp	2	64.38%

Totals
Change from base Build 80:	0.7%
Covered Lines:	37
Relevant Lines:	65

💛 - Coveralls

coveralls · 2018-05-11T12:41:56Z

Pull Request Test Coverage Report for Build 81

0 of 0 (NaN%) changed or added relevant lines in 0 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.7%) to 56.923%

Files with Coverage Reduction	New Missed Lines	%
zmq.hpp	2	64.38%

Totals
Change from base Build 80:	0.7%
Covered Lines:	37
Relevant Lines:	65

💛 - Coveralls

kurdybacha

Good stuff.

kurdybacha · 2018-05-11T17:49:36Z

tests/poller.cpp

@@ -8,117 +7,98 @@

 TEST(poller, create_destroy)
 {
-    zmq::poller_t poller;
-    ASSERT_TRUE(poller.empty ());
+    zmq::poller_t<> poller;


No asserts? :)

I somehow waited for this question ;)

Well, poller_t has no state of its own, that could or would need to be tested. zmq_poller_* do not allow to query anything at the moment. Maybe that would be a good idea, and then an assertion using that could be added here.

kurdybacha · 2018-05-11T17:52:57Z

zmq.hpp

@@ -1019,75 +1018,42 @@ namespace zmq
    };

 #if defined(ZMQ_BUILD_DRAFT_API) && defined(ZMQ_CPP11) && defined(ZMQ_HAVE_POLLER)
+    template <typename T = void>


Is there a need for that? Everything is casted to void* anyway. Besides no tests exercising different types.

It makes it typesafe when adding. active_poller_t uses its handler_t as the template argument. There is at least one test using int as an argument. I think it makes sense.

but poller class is template, not add. There is a use of int in add but with poller<void*> as I can see. What is a point of having poller template as you need to cast it to void* in order to pass to libzmq. It would be ok to have it as template if at least you have a compilation error when trying to pass different type to add than poller template type in use. Right now it does not and your test with int is a proof :)

kurdybacha · 2018-05-11T18:05:26Z

zmq_addon.hpp

+#endif  // ZMQ_HAS_RVALUE_REFS
+
+#if defined(ZMQ_BUILD_DRAFT_API) && defined(ZMQ_CPP11) && defined(ZMQ_HAVE_POLLER)
+    class active_poller_t


Much cleaner now after moving this to separate class. Can we change a name though, please? Something more zmq'ish and shorter? Maybe loop_t, or service_t? I have some pending additions - timers - and it would make sense to put it here. Maybe we can split this actually into separate file? zmq_monitor.hpp, zmq_service.hpp?

I am not completely satisfied with my name as well. I think "poller" should be part of the name, but "active" is not so specific. loop_t would be strange IMO, as there is no loop within wait. A "service" is also something quite different for me, which has its own thread etc. These concepts could be built on top of active_poller_t again.

About splitting up the headers... I am somewhat undecided about that. It is a feature of cppzmq that it consists only of two header files. But on the other hand, when you can copy two header files, you can also copy a handful. If it is done, zmq_addon.hpp should include all the extra headers then.

yeah, naming is the most difficult part in software development. maybe zpoller_t (z - zuper) :)
We can stay with two header for now. If have more stuff we can split then and I like an idea of zmq_addon.hpp to have all , probably with a rename to zmq_addons.hpp

kurdybacha · 2018-05-11T18:09:11Z

zmq_addon.hpp

+        int wait (std::chrono::milliseconds timeout)
+        {
+            if (need_rebuild) {
+                poller_events.resize (handlers.size ());


This is wrong, sorry. resize does not clear items already in the vector so you might pass items from previous run to wait. Depending on this kind of stuff is unreliable and asking for trouble.

That's why I changed this. It is not necessary to clear the items. zmq_poller_wait_all even clears all unused items. But this is unnecessary too. zmq_poller_wait_all returns the number of valid items, and we only access those.

If this were different, you would need to clear all items on every call to wait, regardless of the value of need_rebuild. But this would be quite inefficient if you had a large pollset.

I get a point about performance but my issue was that we rely on internal implementation of libzmq to clear them (set all members) for us. How about if libzmq decides to set only polleritem partially depends on the case e.g. ZMQ_POLLERR etc.. That was my original reason behind clear().

I think the previous version did not clear the members of the event struct either.

We shouldn't rely on implementation details of course, but I don't think we do. We should rely on the specification of the API, which should specify that the indicated number of entries are properly set to be well usable IMO. However, I fear the zmq_poller_* functions are not properly documented yet. I will put this forward on the libzmq side.

Sounds good.

kurdybacha · 2018-05-11T18:38:34Z

zmq_addon.hpp

+        void add (zmq::socket_t &socket, short events, handler_t handler)
+        {
+            auto it = decltype (handlers)::iterator {};
+            auto inserted = bool {};


depending on default initialization here makes it only harder to read.

Hm. I don't think so. Initializing with a value explicitly for which there is no code path where it could ever be read would be confusing IMO.

There is not difference in initializing it explicitly or default. But there is readability and maintainability gain. Explicit initialization makes it harder to break things in the future IMO. Your call here.

I can understand your point of view regarding future changes. I disagree about readability though. If I read some variable is initialized with a specific value I assume this value will be used in some case, and search for a long time if there is no such case.

Anyway, there is no way without some disadvantage. What is more important is to do this consistently throughout cppzmq IMO (I am not sure if there is any other comparable place at the moment). We should prescribe either way in a brief style guide.

Ok. Agree.
We would not have this discussion if we switch active poller to C++17. What do you think about that? Most of the compilers support it now and that would give some more stuff for cleaner and safer code.

I don't think this would be a good idea at the moment. I read that people can't use fbzmq e.g. because it requires C++14 support. For our own uses we will stick to VS2015 for several years to come e.g., which has only very limited support for C++14 or even C++17 features. I think there are also many other environments that at least have no recent gcc/clang by default. We already restrict the platforms quite substantially compared to libzmq itself which does not require C++11.

kurdybacha · 2018-05-11T18:41:23Z

zmq_addon.hpp

+            try 
+            {
+                base_poller.add (socket, events, inserted && *(it->second) ? it->second.get() : nullptr);
+                need_rebuild |= inserted;


why not simply need_rebuild = true ?

In order to avoid an unnecessary rebuild?
IIRC it was like this before, just something like

if (inserted) { need_rebuild = true; ... }

You not avoiding here anything. If you are after base_poller.add you have to rebuild regardless of inserted.

Hm, I think add will throw necessarily if inserted is false, so the need_rebuild line will only be reached if inserted is true anyway. So... it could be changed...

kurdybacha · 2018-05-11T18:43:57Z

zmq_addon.hpp

+                need_rebuild = false;
+            }
+            const int count = base_poller.wait_all (poller_events, timeout);
+            if (count != 0) {


count > 0 , wait_all should return unsigned or size_t.

Yes, I agree, that would be better.

Actually, the condition is completely redundant then. The for_each below just iterates over an empty range if count == 0.

kurdybacha · 2018-05-11T18:46:23Z

tests/active_poller.cpp

+    ASSERT_TRUE(message_received);
+}
+
+/// \todo this contains multiple test cases that should be split up


can we remove this comment?

Do you think it no longer applies?

I think it has never applied. All separate cases are already covered? This is a test that exercises more complex scenario - splinting it up does not make sense because you loose the intend of it.

Ok, then remove it.

sigiesec added 9 commits May 11, 2018 11:02

Problem: poller_t adds an abstraction layer on zmq_poller_*

bf47be0

Solution: extract base_poller_t from poller_t, which provides a direct mapping of zmq_poller_* to C++ only

Problem: move poller tests are unspecific

89d9db7

Solution: remove redundant assertions (tested elsewhere), and add assertion behaviour of calling wait on moved-from poller

Problem: zmq_addon header contains tests (with a syntax error)

dc996cd

Solution: convert to googletest based test, and fix syntax error

Problem: extra abstraction layer type poller_t is in zmq.hpp

882f5e8

Solution: move to zmq_addon.hpp, rename to active_poller_t, and rename base_poller_t to poller_t

Problem: unnecessary heap allocations for test subject

ee1cc9a

Solution: change to local variables

Problem: deprecation warning in multipart_t

f700e5d

Solution: use non-deprecated operator!= instead

Problem: creation of poller_events is unnecessarily complex

3251d05

Solution: simplify code

Problem: no tests for poller_t

762b013

Solution: add tests

Problem: redundant assertion

421d66c

Solution: removed redundant assertion

sigiesec changed the title ~~Split poller layer~~ Split poller_t into two layers May 11, 2018

bluca merged commit 0f840ce into zeromq:master May 11, 2018

sigiesec deleted the split-poller-layer branch May 11, 2018 12:57

kurdybacha reviewed May 11, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split poller_t into two layers #225

Split poller_t into two layers #225

sigiesec commented May 11, 2018

coveralls commented May 11, 2018 •

edited

Loading

coveralls commented May 11, 2018

kurdybacha left a comment

kurdybacha May 11, 2018

sigiesec May 12, 2018

kurdybacha May 11, 2018

sigiesec May 12, 2018

kurdybacha May 12, 2018 •

edited

Loading

kurdybacha May 11, 2018 •

edited

Loading

sigiesec May 12, 2018

kurdybacha May 12, 2018

kurdybacha May 11, 2018

sigiesec May 12, 2018 •

edited

Loading

kurdybacha May 12, 2018

sigiesec May 12, 2018

kurdybacha May 12, 2018

kurdybacha May 11, 2018

sigiesec May 12, 2018

kurdybacha May 12, 2018

sigiesec May 12, 2018

kurdybacha May 12, 2018 •

edited

Loading

sigiesec May 12, 2018

kurdybacha May 11, 2018 •

edited

Loading

sigiesec May 12, 2018

kurdybacha May 12, 2018

sigiesec May 12, 2018

kurdybacha May 11, 2018 •

edited

Loading

sigiesec May 12, 2018

sigiesec May 12, 2018 •

edited

Loading

kurdybacha May 11, 2018

sigiesec May 12, 2018

kurdybacha May 12, 2018 •

edited

Loading

sigiesec May 12, 2018

Split poller_t into two layers #225

Split poller_t into two layers #225

Conversation

sigiesec commented May 11, 2018

coveralls commented May 11, 2018 • edited Loading

Pull Request Test Coverage Report for Build 81

💛 - Coveralls

coveralls commented May 11, 2018

Pull Request Test Coverage Report for Build 81

💛 - Coveralls

kurdybacha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurdybacha May 12, 2018 • edited Loading

Choose a reason for hiding this comment

kurdybacha May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigiesec May 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurdybacha May 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurdybacha May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurdybacha May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigiesec May 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurdybacha May 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 11, 2018 •

edited

Loading

kurdybacha May 12, 2018 •

edited

Loading

kurdybacha May 11, 2018 •

edited

Loading

sigiesec May 12, 2018 •

edited

Loading

kurdybacha May 12, 2018 •

edited

Loading

kurdybacha May 11, 2018 •

edited

Loading

kurdybacha May 11, 2018 •

edited

Loading

sigiesec May 12, 2018 •

edited

Loading

kurdybacha May 12, 2018 •

edited

Loading