Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[tests] Remove rpc_zmq.py #14419

Merged
merged 1 commit into from Oct 8, 2018

Conversation

Projects
None yet
8 participants
@jnewbery
Copy link
Member

commented Oct 7, 2018

rpc_zmq.py is racy and fails intermittently. Remove that test file and
move the getzmqnotifications RPC test into interface_zmq.py.

[tests] Remove rpc_zmq.py
rpc_zmq.py is racy and fails intermittently. Remove that test file and
move the getzmqnotifications RPC test into interface_zmq.py

@fanquake fanquake added the Tests label Oct 7, 2018

@promag

This comment has been minimized.

Copy link
Member

commented Oct 7, 2018

utACK 42a995a.

@achow101

This comment has been minimized.

Copy link
Member

commented Oct 7, 2018

utACK 42a995a

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2018

Reviewers, this pull request conflicts with the following ones:
  • #14220 (Transaction relay privacy bugfix by sdaftuar)
  • #14060 (ZMQ: add options to configure outbound message high water mark, aka SNDHWM by mruddy)

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.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Oct 7, 2018

Why is the race not a bug that should be fixed?

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Oct 7, 2018

Let's get rid of the racy test now and have it re-added as soon as a raciness has been fixed

I can't follow this logic.

As far as I can tell the operative test is added to the interfaces, so there would be nothing to restore.

Moving the test to another file seems reasonable (and faster!).

I'm not clear on why this test is racy.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Oct 7, 2018

@gmaxwell Sorry, my logic was incorrect :-) I incorrectly assumed that some racy subset of the test was being temporarily removed. I now understand that's not the case. A brainfart on my part :-)

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2018

I'm not clear on why this test is racy.

Here's a possibility, although can't demonstrate without the log files:

  • bitcoind is started in rpc_zmq.py L30
  • the rpc server starts in init.cpp L1232 (AppInitServers())
  • there's then a race between:
    a) the test framework calling getzmqnotifications (rpc_zmq.py L31)
    b) the ZMQNotificationInterface being initialized in CZMQNotificationInterface::Create() (init.cpp L1339)

If (a) wins, then getzmqnotifications will return an empty object. See zmqrpc.cpp L36:

    if (g_zmq_notification_interface != nullptr) {
    ...
    }

Why is the race not a bug that should be fixed?

Not a bug. Just the test trying calling getzmqnotifications before the interface has been initialized.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Oct 8, 2018

Not a bug. Just the test trying calling getzmqnotifications before the interface has been initialized.

How is this not a bug. The call should block and wait until the interface has been initialized? Similarly on how we block the wallet on calls until it has caught up with the chainstate, this call should block until all notifications have been activated/deactivated immediately prior to this call.

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2018

How is this not a bug.

The help text for getzmqnotifications says "Returns information about the active ZeroMQ notifications." If you call the RPC before the interface is active, then it (correctly) returns an empty object since no notifications are active.

This is a little bit pedantic though. This is a tiny window condition which as far as I'm aware can only be hit in Travis which has weird scheduling and delays. I don't think this can cause any problems for end-users actually using the zmq interface.

I opened this PR to unblock #14336, which I think is a far more important goal than making sure that the zmq interface gets initialized before getzmqnotifications returns. If you want to fix that, by all means go ahead, but I don't think doing so should block #14336.

@MarcoFalke MarcoFalke merged commit 42a995a into bitcoin:master Oct 8, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Oct 8, 2018

Merge #14419: [tests] Remove rpc_zmq.py
42a995a [tests] Remove rpc_zmq.py (John Newbery)

Pull request description:

  rpc_zmq.py is racy and fails intermittently. Remove that test file and
  move the getzmqnotifications RPC test into interface_zmq.py.

Tree-SHA512: 666c8f252f8a392deda1bd531e84fdc04bdae4eab09407657ade2b5fc0aeffa247735e20314236f56e4e3402476673f3b7538d6e09f5af6976021ba2377ce63c

@jnewbery jnewbery deleted the jnewbery:remove_racy_zmq_test branch Oct 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.