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.

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

Feature/expose monitor socket for active poller #612

Merged

Conversation

dietelTiMaMi
Copy link
Contributor

Motivation:
zmq::monitor_t in it's current form is inconvenient to use in combination with zmq::active_poller_t
the issue is, that the socket isn't available to the user of zmq::monitor_t

Proposal:
split zmq::monitor_t::check_event() to move the actual reading and processing of the events into a new protected method zmq::monitor_t::process_event()
provide a protected socket_ref for using the monitor_socket for polling in a derived class.

Execution:
The proposed changes were done tozmq.hpp
A test case demonstrating the desired usage was added to tests/monitor.cpp

By inheriting from zmq::monitor_t you can now implement custom ways of polling for socket events.
The modified api is hidden for "rookie" users that might accidentaly call process_event() without polling for changes before use.

@dietelTiMaMi
Copy link
Contributor Author

Please restart the check, ran on my fork aftrer removing the chrono literals.
they are not available in C++11.
It seems the Ubuntu 18 runners are no longer existent

@dietelTiMaMi
Copy link
Contributor Author

@gummif Could you please relaunch the CI tests?
in the first run there was some C++14 code that didn't compile in C++11 inside the test
that is now fixed.

I also ran the CI in my fork and except for the Ubuntu 18.04 runners that are no longer existent everything went through.

@dietelTiMaMi
Copy link
Contributor Author

@gummif

Thanks for restarting the workflow.
Do you see any problems for merging this?
If you would like to have any changes, I will gladly contribute to be able to use it in our project.

@gummif
Copy link
Member

gummif commented Sep 6, 2023

Hi I will try to take a look today.

tests/monitor.cpp Outdated Show resolved Hide resolved
@gummif
Copy link
Member

gummif commented Sep 6, 2023

You decide if the comment is worth fixing, otherwise looks good.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6106326776

  • 35 of 70 (50.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.7%) to 86.965%

Changes Missing Coverage Covered Lines Changed/Added Lines %
zmq.hpp 35 70 50.0%
Totals Coverage Status
Change from base Build 5678621379: 0.7%
Covered Lines: 854
Relevant Lines: 982

💛 - Coveralls

@gummif gummif merged commit 85a14af into zeromq:master Sep 8, 2023
10 of 13 checks passed
@dietelTiMaMi dietelTiMaMi deleted the feature/ExposeMonitorSocketForActivePoller branch September 27, 2023 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants