Fix ZMQ Notification initialization and shutdown #6927

promag commented at 6:16 PM on November 1, 2015: member

No description provided.

laanwj commented at 2:39 AM on November 2, 2015: member

Can you explain what is the issue that this is fixing, and how it fixes it?

in src/zmq/zmqnotificationinterface.cpp:None in 7695f9a73a outdated

 103 | @@ -99,7 +104,7 @@ bool CZMQNotificationInterface::Initialize()
 104 |          return false;
 105 |      }
 106 |  
 107 | -    return false;
 108 | +    return true;

promag commented at 10:22 AM on November 2, 2015:

@laanwj This was wrong.

in src/init.cpp:None in 7695f9a73a outdated

1174 | @@ -1176,7 +1175,6 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
1175 |      pzmqNotificationInterface = CZMQNotificationInterface::CreateWithArguments(mapArgs);
1176 |  
1177 |      if (pzmqNotificationInterface) {
1178 | -        pzmqNotificationInterface->Initialize();

promag commented at 10:22 AM on November 2, 2015:

@laanwj The return value was discarded.

promag commented at 10:24 AM on November 2, 2015:

@laanwj Usage in init.cpp is simpler.

in src/zmq/zmqnotificationinterface.cpp:None in 7695f9a73a outdated

  57 | @@ -59,6 +58,12 @@ CZMQNotificationInterface* CZMQNotificationInterface::CreateWithArguments(const
  58 |      {
  59 |          notificationInterface = new CZMQNotificationInterface();
  60 |          notificationInterface->notifiers = notifiers;
  61 | +
  62 | +        if (!notificationInterface->Initialize())

promag commented at 10:23 AM on November 2, 2015:

@laanwj Initialisation is implicit and properly handled.

in src/zmq/zmqnotificationinterface.cpp:None in 7695f9a73a outdated

  20 | @@ -21,8 +21,7 @@ CZMQNotificationInterface::CZMQNotificationInterface() : pcontext(NULL)
  21 |  
  22 |  CZMQNotificationInterface::~CZMQNotificationInterface()
  23 |  {
  24 | -    // ensure Shutdown if Initialize is called
  25 | -    assert(!pcontext);
  26 | +    Shutdown();

promag commented at 10:24 AM on November 2, 2015:

@laanwj Shutdown is implicit.

in src/zmq/zmqnotificationinterface.h:None in 7695f9a73a outdated

  18 | @@ -19,10 +19,11 @@ class CZMQNotificationInterface : public CValidationInterface
  19 |  
  20 |      static CZMQNotificationInterface* CreateWithArguments(const std::map<std::string, std::string> &args);
  21 |  
  22 | +protected:
  23 |      bool Initialize();

promag commented at 10:25 AM on November 2, 2015:

@laanwj No need to expose these since they are called implicitly.

laanwj commented at 11:31 AM on November 2, 2015: member

Agree re: Initialize/Shutdown. As Initialize is always immediately called after the constructor and Shutdown before the destructor it makes little sense to require these to be called explicitly (and have it possible for the object to be in 'created but uninitialized' state.

promag commented at 11:33 AM on November 2, 2015: member

@laanwj :+1:

jonasschnelli commented at 7:28 PM on November 3, 2015: contributor

Codereview ACK. Makes sense to use the destructor instead init.cpp pollution. This also fixes a possible tiny-shutdown-only memory leak in case of something went wrong during initialization.

laanwj commented at 10:33 AM on November 4, 2015: member

utACK.

Please do include more information in your commit message. With an empty description, someone browsing git in the future will not be able to understand the reasoning behind your change.

laanwj added the label Refactoring on Nov 4, 2015

Fix ZMQ Notification initialization and shutdown

Moves the call Initialize() from init.cpp to CreateWithArguments() and handles the
return value. Moves the call Shutdown() from init.cpp to destructor.
Changes Initialize() and Shutdown() to protected members.

de0499d3b8

promag force-pushed on Nov 4, 2015

promag commented at 10:54 AM on November 4, 2015: member

@laanwj :ok_hand:

laanwj merged this on Nov 4, 2015

laanwj closed this on Nov 4, 2015

laanwj referenced this in commit aa03fb35c4 on Nov 4, 2015

luke-jr referenced this in commit e63528ce07 on Nov 27, 2015

zkbot referenced this in commit 36df5a92f8 on Feb 9, 2017

zkbot referenced this in commit dd8b38316f on Feb 9, 2017

zkbot referenced this in commit 253c610783 on Feb 9, 2017

DrahtBot locked this on Sep 8, 2021