ZMQ: Small cleanups in the ZMQ code #13686

domob1812 commented at 11:19 AM on July 17, 2018: contributor

This contains various small code cleanups that make the ZMQ code easier to read and maintain (at least in my opinion). The only functional change is that a potential memory leak is fixed that would have occured when a notifier is removed from the notifiers list after its callback function returned false (which is likely not relevant in practice but still a bug).

domob1812 commented at 11:20 AM on July 17, 2018: contributor

I split the patch into three commits which I think make sense logically and for the review. But of course I'm happy to squash any of the changes, or to revert parts of them (as code readability is a matter of taste).

fanquake added the label RPC/REST/ZMQ on Jul 17, 2018

promag commented at 3:56 PM on July 24, 2018: member

Concept ACK.

From a quick view LGTM.

DrahtBot commented at 12:31 PM on July 27, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19572 (ZMQ: Create "sequence" notifier, enabling client-side mempool tracking by instagibbs)
#18309 (zmq: Add support to listen on multiple interfaces by n-thumann)

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.

DrahtBot added the label Needs rebase on Aug 8, 2018

domob1812 force-pushed on Aug 8, 2018

domob1812 commented at 4:16 PM on August 8, 2018: contributor

Rebased

DrahtBot removed the label Needs rebase on Aug 8, 2018

kostyantyn commented at 10:00 PM on August 20, 2018: contributor

What do you think about wrapping the code with zmq namespace and removing ZMQ/zmq prefix from class/function names? Basically when the location of the file is a/b/c.cpp then we have:

namespace a {
namespace b {
class C {}
}
}

domob1812 commented at 5:59 AM on August 21, 2018: contributor

@kostyantyn: At least for now, there seems to be not much namespace usage at all in the Bitcoin code - so I'm not sure if we should change that lightly. Also, note that zmq is the namespace that is also used by ZMQ's own C++ language bindings; they are currently not used, but I proposed to change that (#13957).

Also, note that C++ namespaces are different from Java's, so that some people advise against nesting (e.g. exact correspondence to the directory structure as you suggest).

laanwj commented at 11:38 AM on August 31, 2018: member

Concept ACK @jmcorgan do you mind taking a look here?

in src/zmq/zmqabstractnotifier.h:12 in 55eca083fa outdated

   4 | @@ -5,12 +5,13 @@
   5 |  #ifndef BITCOIN_ZMQ_ZMQABSTRACTNOTIFIER_H
   6 |  #define BITCOIN_ZMQ_ZMQABSTRACTNOTIFIER_H
   7 |  
   8 | -#include <zmq/zmqconfig.h>
   9 | +#include <memory>
  10 |

ken2812221 commented at 7:02 AM on September 14, 2018:

Missing a bunch of includes here. See appveyor CI result.

domob1812 commented at 10:04 AM on November 6, 2018:

I missed that comment - I've now revised the commit to include <string>. There are still CI errors, though, which I don't understand so far. Has anyone an insight of where they come from? I'll try to resolve them now.

DrahtBot added the label Needs rebase on Nov 5, 2018

domob1812 force-pushed on Nov 5, 2018

domob1812 commented at 2:24 PM on November 5, 2018: contributor

Rebased

DrahtBot removed the label Needs rebase on Nov 5, 2018

domob1812 force-pushed on Nov 5, 2018

domob1812 force-pushed on Nov 6, 2018

in src/zmq/zmqpublishnotifier.cpp:22 in 95c442d496 outdated

  19 |  
  20 | +#include <zmq.h>
  21 | +
  22 | +#include <cstdarg>
  23 | +#include <cstddef>
  24 | +

ken2812221 commented at 9:23 AM on November 9, 2018:

I would suggest adding the following code for Windows.

#if defined(WIN32) && defined(SendMessage)
#undef SendMessage
#endif

Windows headers have defined SendMessage as SendMessageA or SendMessageW

Or you could rename the method name SendMessage.

domob1812 commented at 8:08 PM on November 10, 2018:

Thanks, very interesting! From a quick look, I didn't see what my PR changes here and why it worked before - but I guess a defensive rename of the method is not a bad idea. I did that now in a separate commit and pushed it, just to see if it fixes the CI issues. If it does, I can include this directly in one of the commits or perhaps just keep it as it is.

domob1812 commented at 5:55 PM on November 11, 2018:

Indeed, looks like the tests pass now.

in src/zmq/zmqnotificationinterface.cpp:9 in 5d66a3be2d outdated

   3 | @@ -4,79 +4,69 @@
   4 |  
   5 |  #include <zmq/zmqnotificationinterface.h>
   6 |  #include <zmq/zmqpublishnotifier.h>
   7 | +#include <zmq/zmqutil.h>
   8 | +
   9 | +#include <zmq.h>

hebasto commented at 3:04 PM on December 10, 2018:

Why is a guard dropped?

#ifdef ENABLE_ZMQ
#include <zmq.h>
#endif

domob1812 commented at 8:34 AM on October 17, 2019:

I think I've replied on this before, but just in case and for the record: The guard that used to be here was redundant, as there is a guard already (e.g. in init.cpp) around zmqnotificationinterface itself.

hebasto changes_requested

hebasto commented at 3:05 PM on December 10, 2018: member

Concept ACK.

domob1812 commented at 9:28 AM on December 11, 2018: contributor

@hebasto: For some reason, Github won't let me reply this directly to your comment above. The guard is not necessary, since it is present already where the src/zmq files themselves are used - e.g. in src/init.cpp.

domob1812 force-pushed on May 2, 2019

domob1812 commented at 8:03 AM on May 2, 2019: contributor

Rebased. Is there anything missing for me to do here? I think this is a real improvement to the quality of the ZMQ code.

fanquake commented at 2:20 AM on June 16, 2019: member

@jmcorgan Are you able to take a look at this?

DrahtBot added the label Needs rebase on Oct 16, 2019

domob1812 force-pushed on Oct 17, 2019

DrahtBot removed the label Needs rebase on Oct 17, 2019

domob1812 force-pushed on Oct 17, 2019

domob1812 commented at 8:35 AM on October 17, 2019: contributor

Rebased. Can I do anything to speed up review / merging of this one? It should not need review by the original author or a ZMQ expert, as this is mostly just "standard" refactoring from manual memory management to the use of proper RAII (e.g. std::unique_ptr).

in src/zmq/zmqnotificationinterface.cpp:118 in f18bc54cab outdated

 114 | @@ -136,45 +115,44 @@ void CZMQNotificationInterface::Shutdown()
 115 |      }
 116 |  }
 117 |  
 118 | -void CZMQNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload)
 119 | +namespace

promag commented at 8:56 AM on October 17, 2019:

f18bc54cab43ef1de8502b8f84b5585ae68af4bc

According to developer notes put { here.

domob1812 commented at 10:42 AM on October 17, 2019:

Done.

in src/zmq/zmqnotificationinterface.cpp:118 in f18bc54cab outdated

 129 | -            i++;
 130 | -        }
 131 | -        else
 132 | -        {
 133 | +template <typename Function>
 134 | +void TryForEachAndRemoveFailed(std::list<std::unique_ptr<CZMQAbstractNotifier>>& notifiers, const Function& func)

promag commented at 8:57 AM on October 17, 2019:

f18bc54cab43ef1de8502b8f84b5585ae68af4bc

Could make this refactor in a separate commit?

domob1812 commented at 11:03 AM on October 17, 2019:

Yes sure, if you prefer. Let's finish the review of the code first, and then I will rework the commit history as desired afterwards (else we might mess up the ongoing review with force-pushes).

hebasto commented at 8:57 AM on August 27, 2020:

69f92ffce9f0d11d9389610e702b99d9038f3fed

Could make this refactor in a separate commit?

Agree with @promag.

instagibbs commented at 2:13 PM on September 15, 2020:

Note for reviewers: I think this is resolved?

in src/zmq/zmqabstractnotifier.h:28 in d10f274a6e outdated

  22 | @@ -21,9 +23,9 @@ class CZMQAbstractNotifier
  23 |      virtual ~CZMQAbstractNotifier();
  24 |  
  25 |      template <typename T>
  26 | -    static CZMQAbstractNotifier* Create()
  27 | +    static std::unique_ptr<CZMQAbstractNotifier> Create()
  28 |      {
  29 | -        return new T();
  30 | +        return std::unique_ptr<CZMQAbstractNotifier>(new T());

promag commented at 9:00 AM on October 17, 2019:

d10f274a6e0291e752e25b2bd99c0472d22a420f

MakeUnique?

domob1812 commented at 10:42 AM on October 17, 2019:

Done.

promag commented at 9:07 AM on October 17, 2019: member

Code review ACK ba8b67aee28623d7fdd9b2053bc1784518a4a8ca, with some comments.

domob1812 commented at 10:43 AM on October 17, 2019: contributor

Thanks for the review! I've addressed your comments in follow-up commits (I do not want to force-push right now and perhaps mess up your ongoing review). I will add them as fixups to the proper commits later when you are done with the review.

I can then also split out the creation of TryForEachAndRemoveFailed into a separate commit if you prefer that.

DrahtBot added the label Needs rebase on Dec 31, 2019

domob1812 force-pushed on Jan 1, 2020

domob1812 commented at 8:02 AM on January 1, 2020: contributor

Rebased

DrahtBot removed the label Needs rebase on Jan 1, 2020

in src/zmq/zmqnotificationinterface.cpp:69 in 69f92ffce9 outdated

  72 | +            notificationInterface.reset();
  73 |          }
  74 |      }
  75 |  
  76 | -    return notificationInterface;
  77 | +    return notificationInterface.release();

hebasto commented at 9:00 AM on August 27, 2020:

69f92ffce9f0d11d9389610e702b99d9038f3fed

If this code is touched to make it easier to read and maintain, may I suggest a simpler approach:

    if (!notifiers.empty()) {
        auto notificationInterface = std::unique_ptr<CZMQNotificationInterface>(new CZMQNotificationInterface());
        notificationInterface->notifiers = std::move(notifiers);

        if (notificationInterface->Initialize()) {
            return notificationInterface.release();
        }
    }

    return nullptr;

?

domob1812 commented at 7:06 AM on September 1, 2020:

Indeed, looks better. I've updated the code accordingly (except I changed the auto notificationInterface = ... line slightly into what I consider even clearer; but let me know if you prefer your suggestion).

in src/zmq/zmqpublishnotifier.cpp:18 in 49e2cb7856 outdated

  18 |  #include <rpc/server.h>
  19 |  
  20 | +#include <zmq.h>
  21 | +
  22 | +#include <cstdarg>
  23 | +#include <cstddef>

hebasto commented at 9:07 AM on August 27, 2020:

#include <cstdarg>
#include <cstddef>
#include <map>
#include <string>
#include <utility>

domob1812 commented at 6:59 AM on September 1, 2020:

Indeed, I've pushed this change together with the two nanonits below (in a single commit).

in src/zmq/zmqpublishnotifier.cpp:7 in 49e2cb7856 outdated

   1 | @@ -2,14 +2,25 @@
   2 |  // Distributed under the MIT software license, see the accompanying
   3 |  // file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 |  
   5 | +#if defined(HAVE_CONFIG_H)
   6 | +#include <config/bitcoin-config.h>
   7 | +#endif

hebasto commented at 9:09 AM on August 27, 2020:

Is this required here as no defines from bitcoin-config.h is used here?

domob1812 commented at 7:01 AM on September 1, 2020:

I guess not, removed.

in src/zmq/zmqutil.h:8 in 49e2cb7856 outdated

   0 | @@ -0,0 +1,10 @@
   1 | +// Copyright (c) 2014-2018 The Bitcoin Core developers
   2 | +// Distributed under the MIT software license, see the accompanying
   3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +#ifndef BITCOIN_ZMQ_ZMQUTIL_H
   6 | +#define BITCOIN_ZMQ_ZMQUTIL_H
   7 | +
   8 | +void zmqError(const char *str);

hebasto commented at 9:11 AM on August 27, 2020:

49e2cb78563df27ff0a5d2ad985e074ee98afcce nanonit:

void zmqError(const char* str);

in src/zmq/zmqutil.cpp:11 in 49e2cb7856 outdated

   6 | +
   7 | +#include <logging.h>
   8 | +
   9 | +#include <zmq.h>
  10 | +
  11 | +void zmqError(const char *str)

hebasto commented at 9:12 AM on August 27, 2020:

49e2cb78563df27ff0a5d2ad985e074ee98afcce nanonit:

void zmqError(const char* str)

hebasto changes_requested

hebasto commented at 9:12 AM on August 27, 2020: member

Approach ACK 14b40f1b7ce7d0b457e163dbda4e81f5105db753

domob1812 commented at 7:07 AM on September 1, 2020: contributor

Thanks for the review, @hebasto, very appreciated! I've addressed your suggestions now with fixup commits. I will now rebase this onto current master, and then go clean up the commit history (including splitting out the refactor as suggested by @promag and you).

domob1812 force-pushed on Sep 1, 2020

domob1812 commented at 8:06 AM on September 1, 2020: contributor

Rebase done, and I've now cleaned up the commit history as well (squashed in the "fixup" commits as appropriate). Also split out the TryForEachAndRemoveFailed refactor into its own commit.

hebasto commented at 8:17 AM on September 6, 2020: member

The first commit 707c4803d5edd2d5ebd6febb33e08bb031630652 is not compiled for me. Make iterator type auto?

domob1812 force-pushed on Sep 7, 2020

Various cleanups in zmqnotificationinterface.

This is a pure refactoring of zmqnotificationinterface to make the
code easier to read and maintain.  It replaces explicit iterators
with C++11 for-each loops where appropriate and uses std::unique_ptr
to make memory ownership more explicit.

e15b1cfc31

Simplify and fix notifier removal on error.

This factors out the common logic to run over all ZMQ notifiers, call a
function on them, and remove them from the list if the function fails is
extracted to a helper method.

Note that this also fixes a potential memory leak:  When a notifier was
removed previously after its callback returned false, it would just be
removed from the list without destructing the object.  This is now done
correctly by std::unique_ptr behind the scenes.

b93b9d5456

Use std::unique_ptr for CZMQNotifierFactory.

Instead of returning a raw pointer from CZMQNotifierFactory and
implicitly requiring the caller to know that it has to take ownership,
return a std::unique_ptr to make this explicit.

This also changes the typedef for CZMQNotifierFactory to use the new
C++11 using syntax, which makes it (a little) less cryptic.

7f2ad1b9ac

Replace zmqconfig.h by a simple zmqutil.

zmqconfig.h is currently not really needed anywhere, except that
it declares zmqError (which is then defined in
zmqnotificationinterface.cpp).  Note in particular that there is
no need to conditionally include zmq.h only if ZMQ is enabled, because
the place in the core code where the ZMQ library itself is included
(init.cpp) is conditional already on that.

This commit removes zmqconfig.h and replaces it by a much simpler
zmqutil.h library for zmqError.  The definition of the function is
moved to the matching (newly created) zmqutil.cpp.

a3ffb6ebeb

domob1812 force-pushed on Sep 7, 2020

domob1812 commented at 9:06 AM on September 7, 2020: contributor

The first commit 707c480 is not compiled for me. Make iterator type auto?

Sorry for that, it is fixed now. The overall PR is not changed (except I've rebased it to current master), but now each individual commit builds.

hebasto changes_requested

hebasto commented at 11:10 AM on September 14, 2020: member

ACK 166ddcbef82ba2b6734e8f09584fb73e4f9f98a4.

Mind making the latest commit 166ddcbef82ba2b6734e8f09584fb73e4f9f98a4 a sripted-diff with the following script:

sed -i 's/SendMessage/SendZmqMessage/g' src/zmq/zmqpublishnotifier.*

?

fanquake requested review from instagibbs on Sep 15, 2020

scripted-diff: Rename SendMessage to SendZmqMessage.

Windows headers define SendMessage as a macro, which leads to problems
with the method name "SendMessage".  To circumvent this, we rename the
method to "SendZmqMessage".

-BEGIN VERIFY SCRIPT-
sed -i 's/SendMessage/SendZmqMessage/g' src/zmq/zmqpublishnotifier.*
-END VERIFY SCRIPT-

6fe2ef2acb

domob1812 force-pushed on Sep 15, 2020

domob1812 commented at 7:40 AM on September 15, 2020: contributor

Mind making the latest commit 166ddcb a sripted-diff with the following script:

Good point, done.

instagibbs commented at 2:23 PM on September 15, 2020: member

utACK 6fe2ef2acb00b1df7f6a0c0dea1a81a1924be0e1

I restarted one travis job that appeared to be a spurious timeout.

hebasto approved

hebasto commented at 2:25 PM on September 15, 2020: member

re-ACK 6fe2ef2acb00b1df7f6a0c0dea1a81a1924be0e1, only the latest commit got a scripted-diff since my previous review.

fanquake merged this on Sep 19, 2020

fanquake closed this on Sep 19, 2020

sidhujag referenced this in commit 99dd8bffb8 on Sep 20, 2020

domob1812 deleted the branch on Sep 21, 2020

domob1812 referenced this in commit 956668abec on Sep 21, 2020

ftrader referenced this in commit 01f5955382 on Apr 14, 2021

kittywhiskers referenced this in commit c7c5ed352e on Aug 28, 2021

PastaPastaPasta referenced this in commit bbc8623245 on Aug 31, 2021

deadalnix referenced this in commit 9d00dd39b5 on Oct 11, 2021

deadalnix referenced this in commit 9a008904ee on Oct 11, 2021

deadalnix referenced this in commit 7a143c3086 on Oct 11, 2021

deadalnix referenced this in commit df5197f20d on Oct 11, 2021

deadalnix referenced this in commit 5fcbf32ac0 on Oct 11, 2021

DrahtBot locked this on Feb 15, 2022