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.

Sign up for GitHub

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

Jump to bottom

Feat/monitor poll #414

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Feat/monitor poll #414

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a4570b1
monitor: add get_event() method
lp35 May 14, 2020
78dab5d
monitor: add compatibility with zmq::poll()
lp35 May 14, 2020
3aa80d0
monitor: add compatibility with zmq::poller_t
lp35 May 14, 2020
c2298e8
monitor: expose close() method to public
lp35 May 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 28 additions & 4 deletions tests/monitor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,12 +124,36 @@ TEST_CASE("monitor init get event count", "[monitor]")
CHECK_FALSE(monitor.get_event(eventMsg, address, zmq::recv_flags::dontwait));
s.init();

while (total < expected_event_count)
SECTION("get_event")
{
if (!monitor.get_event(eventMsg, address))
continue;
while (total < expected_event_count)
{
if (!monitor.get_event(eventMsg, address))
continue;

lbd_count_event(eventMsg);
}

}

SECTION("poll get_event")
{
while (total < expected_event_count)
{
zmq::pollitem_t items[] = {
{ monitor.handle(), 0, ZMQ_POLLIN, 0 },
};

zmq::poll(&items[0], 1, 100);
Copy link
Member

Choose a reason for hiding this comment

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

Please use the overload taking std::chrono::milliseconds.

Copy link
Author

Choose a reason for hiding this comment

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

Roger that


lbd_count_event(eventMsg);
if (!(items[0].revents & ZMQ_POLLIN)) {
continue;
}

CHECK(monitor.get_event(eventMsg, address));

lbd_count_event(eventMsg);
}
}

CHECK(connect_delayed == 1);
Expand Down
8 changes: 8 additions & 0 deletions zmq.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2224,6 +2224,14 @@ class monitor_t
on_monitor_started();
}

operator void *() ZMQ_NOTHROW { return handle(); }

operator void const *() const ZMQ_NOTHROW { return handle(); }
Comment on lines +2227 to +2229
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather not add these operators. socket_t and context_t have them because they are direct correspondents of the underlying C data structures. But that's not the case here, and it's not obvious that a cast to void* returns the monitor socket handle.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed.


ZMQ_NODISCARD void *handle() ZMQ_NOTHROW { return _monitor_socket.handle(); }

ZMQ_NODISCARD const void *handle() const ZMQ_NOTHROW { return _monitor_socket.handle(); }
Comment on lines +2231 to +2233
Copy link
Member

Choose a reason for hiding this comment

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

Similar to the comment above, I'd use more specific names for these functions. We are in monitor_t, so monitor is probably redundant, but i'd use socket_handle.

Also, the const overload should be removed. There are no functions in the C zmq API that accept a const void* socket. I now they exist for other classes, but that's rather a legacy.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed.


#if ZMQ_VERSION_MAJOR >= 4
bool get_event(zmq_event_t& eventMsg, std::string& address, zmq::recv_flags flags = zmq::recv_flags::none)
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure about the signature of this method. What about

std::optional<std::pair<zmq_event_t, std::string>> get_event(zmq::recv_flags flags = zmq::recv_flags::none)

(This requires C++17 this way, but we can also do this with the fallback to detail::trivial_optional as done for other types)

This removes the ambiguity of whether the original value of eventMsg and address are used and in what cases they are modified.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I'm not very familiar with std::optional and will try to implement this way

{
Expand Down