ZMQ: Use C++ language bindings #14017

pull domob1812 wants to merge 1 commits into bitcoin:master from domob1812:zmq-cpp changing 7 files +46 −73
  1. domob1812 commented at 6:57 PM on August 21, 2018: contributor

    This changes the ZMQ code to use the C++ language bindings instead of the plain C interface. This allows us to make use of RAII for initialisation and destruction, making the code easier to read and less error-prone.

    See #13957.

  2. domob1812 commented at 6:58 PM on August 21, 2018: contributor

    There are a couple of other things in the ZMQ code that I think should be cleaned up (see #13686) - this is purely focused on changing to the C++ language bindings, so the effect on code clarity can be judged.

  3. DrahtBot commented at 9:53 PM on August 21, 2018: member

    <!--e57a25ab6845829454e8d69fc972939a-->Note to reviewers: This pull request conflicts with the following ones:

    • #14060 (ZMQ: add options to configure outbound message high water mark, aka SNDHWM by mruddy)
    • #13686 (ZMQ: Small cleanups in the ZMQ code by domob1812)
    • #13578 ([depends, zmq, doc] upgrade zeromq to 4.2.5 and avoid deprecated zeromq api functions by mruddy)
    • #13315 (configurable ZMQ message limit by FeedTheWeb)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  4. in src/zmq/zmqnotificationinterface.cpp:112 in 4216ed36a0 outdated 
     108 | @@ -119,17 +109,11 @@ bool CZMQNotificationInterface::Initialize()
 109 |  void CZMQNotificationInterface::Shutdown()
 110 |  {
 111 |      LogPrint(BCLog::ZMQ, "zmq: Shutdown notification interface\n");
 112 | -    if (pcontext)
 113 | +    for (std::list<CZMQAbstractNotifier*>::iterator i=notifiers.begin(); i!=notifiers.end(); ++i)
    ch4ot1c commented at 10:15 PM on August 21, 2018:

    Nit: Make this and line 86 match

    domob1812 commented at 6:13 AM on August 22, 2018:

    I didn't touch anything beside the C->C++ conversion on purpose, so that this PR can show how that change looks like. A general cleanup (including these loops) is in #13686.

  5. in src/zmq/zmqnotificationinterface.cpp:115 in 4216ed36a0 outdated 
     120 | -        }
 121 | -        zmq_ctx_destroy(pcontext);
 122 | -
 123 | -        pcontext = nullptr;
 124 | +        CZMQAbstractNotifier *notifier = *i;
 125 | +        LogPrint(BCLog::ZMQ, "   Shutdown notifier %s at %s\n", notifier->GetType(), notifier->GetAddress());
    ch4ot1c commented at 10:18 PM on August 21, 2018:

    Nit zmq: is missing as a prefix here. Or, it could be removed everywhere besides zmqError.

    domob1812 commented at 6:15 AM on August 22, 2018:

    I didn't change the existing code here (see my previous comment about the intent of this PR). I think we can make it consistent nevertheless, but I'd rather do this in a separate commit/PR (perhaps as part of #13686). Also, from a quick grep it seems that the styles with and without prefix are pretty 50/50 at the moment.

  6. in src/zmq/zmqpublishnotifier.cpp:164 in 4216ed36a0 outdated 
     160 | @@ -177,7 +161,7 @@ bool CZMQPublishRawBlockNotifier::NotifyBlock(const CBlockIndex *pindex)
 161 |          CBlock block;
 162 |          if(!ReadBlockFromDisk(block, pindex, consensusParams))
 163 |          {
 164 | -            zmqError("Can't read block from disk");
 165 | +            LogPrint(BCLog::ZMQ, "zmq: Can't read block from disk\n");
    ch4ot1c commented at 10:21 PM on August 21, 2018:

    (Related Nit: zmq: is inconsistent)

    domob1812 commented at 6:17 AM on August 22, 2018:

    See my other comment.

  7. in doc/zmq.md:37 in 4216ed36a0 outdated 
      33 | @@ -34,8 +34,8 @@ buffering or reassembly.
  34 |  ## Prerequisites
  35 |  
  36 |  The ZeroMQ feature in Bitcoin Core requires ZeroMQ API version 4.x or
  37 | -newer. Typically, it is packaged by distributions as something like
  38 | -*libzmq3-dev*. The C++ wrapper for ZeroMQ is *not* needed.
  39 | +newer and the C++ langauge bindings.  Typically, it is packaged by
    ch4ot1c commented at 10:21 PM on August 21, 2018:

    Nit: language

    domob1812 commented at 6:17 AM on August 22, 2018:

    What do you think is wrong? Feel free to suggest an alternative wording (I'm not a native speaker).

    promag commented at 8:23 AM on August 22, 2018:

    There is a typo in language, look carefully.

    domob1812 commented at 8:30 AM on August 22, 2018:

    Ah, my bad - fixed! (I interpreted that "language" to mean that something is wrong/bad related to grammar or style.)

  8. ch4ot1c commented at 10:21 PM on August 21, 2018: contributor

    utACK

  9. ch4ot1c changes_requested
  10. fanquake added the label RPC/REST/ZMQ on Aug 21, 2018
  11. domob1812 commented at 6:17 AM on August 22, 2018: contributor

    Thanks for the review!

  12. domob1812 commented at 6:19 AM on August 22, 2018: contributor

    It seems the build fails on Windows in CI due to missing the ZMQ C++ language bindings. I don't know anything about either the CI system or Windows; so not sure if this just means we have to reconfigure it somehow or if that indicates that we need another dependency that is potentially unwanted.

  13. ZMQ: Use C++ language bindings.
    This changes the ZMQ code to use the C++ language bindings instead of the
plain C interface.  This allows us to make use of RAII for initialisation
and destruction, making the code easier to read and less error-prone.

See https://github.com/bitcoin/bitcoin/issues/13957.
    5b263399a0
  14. domob1812 force-pushed on Aug 22, 2018
  15. ken2812221 commented at 10:02 AM on August 22, 2018: contributor

    I can't see zmq.hpp here: https://github.com/zeromq/libzmq/tree/master/include

    Does this require extra dependencies?

  16. domob1812 commented at 11:13 AM on August 22, 2018: contributor

    @ken2812221: This is the C++ language binding for ZMQ. It is included in the same Debian package as the core library, libzmq3-dev. I don't know where in the sources it is, though.

  17. laanwj commented at 11:44 AM on August 22, 2018: member

    ~0 on this if this doesn't require extra dependencies on any platform, it would be slightly preferable to use the C++ bindings from C++, but if it does require extra packages on any distro, I don't think it's worth it

  18. domob1812 commented at 11:51 AM on August 22, 2018: contributor

    @laanwj: I agree - I think this is mainly a good idea if it does not require extra dependencies. For Debian (which is my system), it does not - but I don't know how it is on other systems.

  19. ken2812221 commented at 2:54 AM on August 23, 2018: contributor

    This seems to be at https://github.com/zeromq/cppzmq/blob/master/zmq.hpp, so it requires an extra dependency. (For depends build)

  20. laanwj commented at 10:38 AM on August 27, 2018: member

    Ok, closing in that case.

  21. laanwj closed this on Aug 27, 2018
  22. domob1812 deleted the branch on Aug 28, 2018
  23. DrahtBot locked this on Sep 8, 2021
Contributors
domob1812DrahtBotch4ot1cpromagken2812221laanwj
Labels
RPC/REST/ZMQ
Linked (view graph)
#13957 Use C++ language binding for ZMQ
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me