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: Allow closed rpc handler in assert_start_raises_init_error #14413

Merged
merged 1 commit into from Oct 8, 2018

Conversation

Projects
None yet
5 participants
@ken2812221
Copy link
Member

commented Oct 6, 2018

The rpc handler may be unregistered when http server haven't been closed yet. So it may be allowable to get -342 non-JSON HTTP response with \'%i %s\' from server (503 Service Unavailable)

See https://ci.appveyor.com/project/DrahtBot/bitcoin/build/master.2001. It shows "Rejecting request while shutting down" between "RPC stopped" and "Stopped HTTP server"

@fanquake fanquake added the Tests label Oct 6, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:test-uacomment-log branch 2 times, most recently Oct 6, 2018

@practicalswift

This comment has been minimized.

Copy link
Member

commented Oct 7, 2018

Concept ACK

I've seen this in AppVeyor. Thanks for addressing!

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2018

Code change seems reasonable, but I don't understand the issue.

What would cause the RPC handler to become unregistered while the test framework is in wait_for_rpc_connection? Why would the nodes be getting shut down if the test framework isn't shutting them down?

test/functional/test_framework/test_node.py Outdated
if e.error['code'] == -28: # RPC in warmup?
pass
elif e.error['code'] == -342: # RPC handler unregistered, but http server haven't been stopped yet.
raise FailedToStartError(self._node_msg(e.error['message']))

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Oct 8, 2018

Member

Maybe just pass this on as well? This way we'd make sure to always have an exit code as opposed to randomly this message or an exit code due to the racyness.

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Oct 8, 2018

Author Member

Sure, fixed

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Oct 8, 2018

@ryanofsky If the node can't initialize itself shuts down itself without having the test framework shut it down.

@ryanofsky
Copy link
Contributor

left a comment

@ryanofsky If the node can't initialize itself shuts down itself without having the test framework shut it down.

Thanks. I didn't realize assert_start_raises_init_error would wait for RPCs to be ready. But I guess it needs to in order to call shut down the node when the error doesn't trigger.

test/functional/test_framework/test_node.py Outdated
if e.error['code'] != -28: # RPC in warmup?
if e.error['code'] == -28: # RPC in warmup?
pass
elif e.error['code'] == -342: # RPC handler unregistered, but http server haven't been stopped yet.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 8, 2018

Contributor

I think this comment is confusing without mentioning why the server is shutting down. Maybe change it to something like:

# Service unavailable, RPC server started but is shutting down due to error

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Oct 8, 2018

Author Member

Comment added

test/functional/test_framework/test_node.py Outdated
if e.error['code'] == -28: # RPC in warmup?
pass
elif e.error['code'] == -342: # RPC handler unregistered, but http server haven't been stopped yet.
raise FailedToStartError(self._node_msg(e.error['message']))

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 8, 2018

Contributor

Would it be good to call self.process.wait() here and include the exit code in the exception, so this is more similar to the previous FailedToStartError exception above?

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Oct 8, 2018

Author Member

I would prefer @MarcoFalke's method, then we don't have to raise it here again.

@ken2812221 ken2812221 force-pushed the ken2812221:test-uacomment-log branch Oct 8, 2018

@ken2812221 ken2812221 force-pushed the ken2812221:test-uacomment-log branch to 62c304e Oct 8, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Oct 8, 2018

utACK 62c304e

@ryanofsky
Copy link
Contributor

left a comment

utACK 62c304e

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

Merge bitcoin#14413: tests: Allow closed rpc handler in assert_start_…
…raises_init_error

62c304e tests: Allow closed http server in assert_start_raises_init_error (Chun Kuan Lee)

Pull request description:

  The rpc handler may be unregistered when http server haven't been closed yet. So it may be allowable to get -342 `non-JSON HTTP response with \'%i %s\' from server` (503 Service Unavailable)

  See https://ci.appveyor.com/project/DrahtBot/bitcoin/build/master.2001. It shows "Rejecting request while shutting down" between "RPC stopped" and "Stopped HTTP server"

Tree-SHA512: e1f50ab9096cf23494ccc2850c01516c4a75f112c99108759157b22fce2011682a4b88216f206702f6a56e960182c00098053ad75f13aa0eafe27046adae63da

@MarcoFalke MarcoFalke merged commit 62c304e into bitcoin:master Oct 8, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ken2812221 ken2812221 deleted the ken2812221:test-uacomment-log 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.