Skip to content

Feat/monitor poll #414

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

Open
lp35 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
18 changes: 18 additions & 0 deletions tests/monitor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,24 @@ TEST_CASE("monitor init get event count", "[monitor]")
}
}

SECTION("poller_t get_event")
{
zmq::poller_t<> poller;
CHECK_NOTHROW(poller.add(monitor, zmq::event_flags::pollin));

while (total < expected_event_count)
{
std::vector<zmq::poller_event<>> events(1);
if(0 == poller.wait_all(events, std::chrono::milliseconds{ 100 }))
continue;

CHECK(zmq::event_flags::pollin == events[0].events);
CHECK(monitor.get_event(eventMsg, address));

lbd_count_event(eventMsg);
}
}

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

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

@sigiesec sigiesec May 22, 2020

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
Copy Markdown
Author

@lp35 lp35 May 22, 2020

Choose a reason for hiding this comment

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

Agreed.


operator socket_ref() ZMQ_NOTHROW { return (zmq::socket_ref) _monitor_socket; }
Copy link
Copy Markdown
Member

@sigiesec sigiesec May 22, 2020

Choose a reason for hiding this comment

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

Again, I'd not add an operator here but rather a named socket() member function. Actually, then the handle/socket_handle function is not necessary at all, since its handle can be accessed through the socket_ref.

Copy link
Copy Markdown
Author

@lp35 lp35 May 22, 2020

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
Copy Markdown
Member

@sigiesec sigiesec May 22, 2020

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
Copy Markdown
Author

@lp35 lp35 May 22, 2020

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