bugfix: fix initialization order bugs

[glevco]

Acceptance Criteria:

• Fix bug that was preventing node initialization due to incorrect import order
  • The Event Simulator indirectly imported code that used get_settings before it was called by run_node with the correct configuration, therefore generating a loading config twice with a different file error.
• Fix bug that was preventing node initialization with the events feature due to incorrect start order
  • The HathorManager was sending events relevant to the EventManager before starting it.

[codecov]

Codecov Report

Merging #550 (551182f) into dev (551182f) will not change coverage.
The diff coverage is n/a.

❗ Current head 551182f differs from pull request most recent head 406eb93. Consider uploading reports for the commit 406eb93 to get more accurate results

@@           Coverage Diff           @@
##              dev     #550   +/-   ##
=======================================
  Coverage   83.19%   83.19%           
=======================================
  Files         211      211           
  Lines       18665    18665           
  Branches     2593     2593           
=======================================
  Hits        15528    15528           
  Misses       2565     2565           
  Partials      572      572           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[jansegre]

The cli import fix is good as is, about the other one I wonder if we should make a test for it.

[glevco]

The cli import fix is good as is, about the other one I wonder if we should make a test for it.

@jansegre I was investigating this and I found out that existing tests in test_event_manager.py were supposed to have caught this. The code in PubSub for publishing events is:

    def publish(self, key: HathorEvents, **kwargs: Any) -> None:
        reactor_thread = ReactorThread.get_current_thread(self.reactor)

        args = EventArguments(**kwargs)
        for fn in self._subscribers[key]:
            if reactor_thread == ReactorThread.NOT_RUNNING:
                fn(key, args)
            else:
                is_empty = bool(not self.queue)
                self.queue.append((fn, key, args))
                if is_empty:
                    self._schedule_call_next()

The reason the tests passed is because tests use the Clock class instead of the reactor, so incidentally things are called in the correct order: the else branch is called and the event handler function call is delayed to after the EventManager is started. In the real case, when running the full node, the if reactor_thread == ReactorThread.NOT_RUNNING: branch is called, and therefore the event handler function is called before the EventManager is started.

Also, ReactorThread is where the MAIN_THREAD value is returned for Clock (for testing):

    def get_current_thread(cls, reactor: Reactor) -> 'ReactorThread':
        """ Returns if the code is being run on the reactor thread, if it's running already.
        """
        running = getattr(reactor, 'running', None)
        if running is not None:
            if running:
                return cls.MAIN_THREAD if isInIOThread() else cls.NOT_MAIN_THREAD
            else:
                # if reactor is not running yet, there's no threading
                return cls.NOT_RUNNING
        else:
            # on tests, we use Clock instead of a real Reactor, so there's
            # no threading. We consider that the reactor is running
            return cls.MAIN_THREAD

I feel that we should somehow either change ReactorThread or PubSub themselves, to prevent this from happening again in the future. Code logic is getting executed differently in production and in tests for this reason.

For example, changing the above last line from return cls.MAIN_THREAD to return cls.NOT_RUNNING would fix the problem, but I'm not sure of other unintended consequences. Edit: I have tested this, and it breaks a lot of other tests. Which may be a bit weird because maybe it indicates that there's code that relies on the specific implementation of PubSub and the order it is going to call message handlers (either at the time of a publish, or the postponed call)...

What do you think?

[jansegre]

What do you think?

I think we should open an issue and investigate this separately and independently from this fix. I'd say this PR can me merged as is.

[glevco]

I think we should open an issue and investigate this separately and independently from this fix

I have created an issue: #552