ZMQ: Small cleanups in the ZMQ code #13686

domob1812 · 2018-07-17T11:19:48Z

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 · 2018-07-17T11:20:47Z

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).

promag · 2018-07-24T15:56:16Z

Concept ACK.

From a quick view LGTM.

DrahtBot · 2018-07-27T12:31:55Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

ZMQ: Create "sequence" notifier, enabling client-side mempool tracking #19572 (ZMQ: Create "sequence" notifier, enabling client-side mempool tracking by instagibbs)
zmq: Add support to listen on multiple interfaces #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.

domob1812 · 2018-08-08T16:16:10Z

Rebased

kostyantyn · 2018-08-20T22:00:15Z

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 · 2018-08-21T05:59:43Z

@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 · 2018-08-31T11:38:00Z

Concept ACK

@jmcorgan do you mind taking a look here?

src/zmq/zmqabstractnotifier.h

domob1812 · 2018-11-05T14:24:08Z

Rebased

src/zmq/zmqpublishnotifier.cpp

hebasto

Concept ACK.

src/zmq/zmqnotificationinterface.cpp

domob1812 · 2018-12-11T09:28:35Z

@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 · 2019-05-02T08:03:41Z

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 · 2019-06-16T02:20:14Z

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

domob1812 · 2020-09-01T08:06:00Z

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 · 2020-09-06T08:17:30Z

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

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.

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.

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.

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.

domob1812 · 2020-09-07T09:06:24Z

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

ACK 166ddcb.

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

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

?

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-

domob1812 · 2020-09-15T07:40:16Z

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

Good point, done.

instagibbs · 2020-09-15T14:23:58Z

utACK 6fe2ef2

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

hebasto

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

6fe2ef2 scripted-diff: Rename SendMessage to SendZmqMessage. (Daniel Kraft) a3ffb6e Replace zmqconfig.h by a simple zmqutil. (Daniel Kraft) 7f2ad1b Use std::unique_ptr for CZMQNotifierFactory. (Daniel Kraft) b93b9d5 Simplify and fix notifier removal on error. (Daniel Kraft) e15b1cf Various cleanups in zmqnotificationinterface. (Daniel Kraft) Pull request description: 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). ACKs for top commit: instagibbs: utACK 6fe2ef2 hebasto: re-ACK 6fe2ef2, only the latest commit got a scripted-diff since my [previous](bitcoin#13686 (review)) review. Tree-SHA512: 8206f8713bf3698d7cd4cb235f6657dc1c4dd920f50a8c5f371a559dd17ce5ab6d94d6281165eef860a22fc844a6bb25489ada12c83ebc780efd7ccdc0860f70

Merged bitcoin/bitcoin#13686 with Xaya's ZMQ extensions.

Co-authored-by: Calin Culianu <calin.culianu@gmail.com> Summary --- This is a backport of [Core#13686](bitcoin/bitcoin#13686). While working on !1045 I noticed some un-idiomatic C++ usage and a potential memory leak. I went and looked at the Core repo and it turns out they have since updated the ZMQ subsystem to be better and they fixed the leak. Original commit message: 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). Note that this depends on MR !1045 (it is a commit on top of it). I adapted this code a little bit to alphabetize the headers and also other minor tiny nits. It's 99% the Core commit, essentially. Test Plan --- - `ninja all check` - `test/functional/test_runner.py interface_zmq` - `test/functional/test_runner.py rpc_zmq`

merge bitcoin#18820, bitcoin#19764, bitcoin#13686, bitcoin#17538, bitcoin#18405: zmq backports, boost depends split

Summary: 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. This is a backport of [[bitcoin/bitcoin#13686 | core#13686]] [1/5] bitcoin/bitcoin@e15b1cf Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10282

Summary: 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. This is a backport of [[bitcoin/bitcoin#13686 | core#13686]] [2/5] bitcoin/bitcoin@b93b9d5 Depends on D10282 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10283

Summary: 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. This is a backport of [[bitcoin/bitcoin#13686 | core#13686]] [3/5] bitcoin/bitcoin@7f2ad1b Depends on D10283 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10284

Summary: 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. This is a backport of [[bitcoin/bitcoin#13686 | core#13686]] bitcoin/bitcoin@a3ffb6e See D7579 for why blockdb.h is included instead of validation.h in zmqpublishnotifier.cpp Depends on D10284 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10285

Summary: 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". This is a backport of [[bitcoin/bitcoin#13686 | core#13686]] [5/5] bitcoin/bitcoin@6fe2ef2 Depends on D10285 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10286

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

DrahtBot mentioned this pull request Jul 27, 2018

0.17: Pre-branch maintenance #13780

Merged

DrahtBot added the Needs rebase label Aug 8, 2018

domob1812 force-pushed the zmq-cleanup branch from 6606c74 to 55eca08 Compare August 8, 2018 16:15

DrahtBot removed the Needs rebase label Aug 8, 2018

DrahtBot mentioned this pull request Aug 20, 2018

refactor: Removal of circular dependency between index/txindex, validation and index/base #13942

Closed

domob1812 mentioned this pull request Aug 21, 2018

ZMQ: Use C++ language bindings #14017

Closed

DrahtBot mentioned this pull request Aug 25, 2018

ZMQ: add options to configure outbound message high water mark, aka SNDHWM #14060

Merged

ken2812221 reviewed Sep 14, 2018

View reviewed changes

src/zmq/zmqabstractnotifier.h Show resolved Hide resolved

DrahtBot added the Needs rebase label Nov 5, 2018

domob1812 force-pushed the zmq-cleanup branch from 55eca08 to 00447bc Compare November 5, 2018 14:23

DrahtBot removed the Needs rebase label Nov 5, 2018

domob1812 force-pushed the zmq-cleanup branch 2 times, most recently from c4ce449 to 95c442d Compare November 6, 2018 08:18

ken2812221 reviewed Nov 9, 2018

View reviewed changes

src/zmq/zmqpublishnotifier.cpp Show resolved Hide resolved

hebasto requested changes Dec 10, 2018

View reviewed changes

src/zmq/zmqnotificationinterface.cpp Show resolved Hide resolved

domob1812 force-pushed the zmq-cleanup branch from 5d66a3b to 02464dd Compare May 2, 2019 07:54

DrahtBot added the Needs rebase label Oct 16, 2019

domob1812 force-pushed the zmq-cleanup branch from 02464dd to f7950b1 Compare October 17, 2019 05:42

domob1812 force-pushed the zmq-cleanup branch from b43ad20 to d47b028 Compare September 1, 2020 07:42

domob1812 force-pushed the zmq-cleanup branch from d47b028 to 7da6bad Compare September 7, 2020 08:46

domob1812 added 4 commits September 7, 2020 10:55

Various cleanups in zmqnotificationinterface.

e15b1cf

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.

domob1812 force-pushed the zmq-cleanup branch from 7da6bad to 166ddcb Compare September 7, 2020 08:57

hebasto requested changes Sep 14, 2020

View reviewed changes

fanquake requested a review from instagibbs September 15, 2020 06:45

domob1812 force-pushed the zmq-cleanup branch from 166ddcb to 6fe2ef2 Compare September 15, 2020 07:40

hebasto approved these changes Sep 15, 2020

View reviewed changes

fanquake merged commit 831b0ec into bitcoin:master Sep 19, 2020

domob1812 deleted the zmq-cleanup branch September 21, 2020 07:48

domob1812 added a commit to xaya/xaya that referenced this pull request Sep 21, 2020

Merge branch 'namecoin'

956668a

Merged bitcoin/bitcoin#13686 with Xaya's ZMQ extensions.

kwvg added a commit to kwvg/dash that referenced this pull request Aug 28, 2021

merge bitcoin#13686: Small cleanups in the ZMQ code

c7c5ed3

PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Aug 31, 2021

Merge pull request #4356 from kittywhiskers/depends

bbc8623

merge bitcoin#18820, bitcoin#19764, bitcoin#13686, bitcoin#17538, bitcoin#18405: zmq backports, boost depends split

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

ZMQ: Small cleanups in the ZMQ code #13686

ZMQ: Small cleanups in the ZMQ code #13686

Uh oh!

Conversation

domob1812 commented Jul 17, 2018

Uh oh!

domob1812 commented Jul 17, 2018

Uh oh!

promag commented Jul 24, 2018

Uh oh!

DrahtBot commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

domob1812 commented Aug 8, 2018

Uh oh!

kostyantyn commented Aug 20, 2018

Uh oh!

domob1812 commented Aug 21, 2018

Uh oh!

laanwj commented Aug 31, 2018

Uh oh!

Uh oh!

domob1812 commented Nov 5, 2018

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

domob1812 commented Dec 11, 2018

Uh oh!

domob1812 commented May 2, 2019

Uh oh!

fanquake commented Jun 16, 2019

Uh oh!

domob1812 commented Sep 1, 2020

Uh oh!

hebasto commented Sep 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domob1812 commented Sep 7, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

domob1812 commented Sep 15, 2020

Uh oh!

instagibbs commented Sep 15, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Jul 27, 2018 •

edited

Loading

hebasto commented Sep 6, 2020 •

edited

Loading