Skip to content

Conversation

@holiman
Copy link
Contributor

@holiman holiman commented Feb 11, 2021

This PR does two things:

  • If geth --dev --ws --ws.port 0 --http --http.port 0 is used, then ws and http are both randomized, to different ports.
  • If geth --dev --ws --ws.port 9999 --http --http.port 9999, then it shows in the logs that, yes, websocket is up, even if it's on the same port.
INFO [02-11|10:00:53.477] WebSocket enabled                        url=ws://127.0.0.1:9999
INFO [02-11|10:00:53.477] HTTP server started                      endpoint=127.0.0.1:9999 prefix= cors= vhosts=localhost

@holiman holiman requested review from fjl and renaynay as code owners February 11, 2021 09:01
Copy link
Contributor

@renaynay renaynay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it out and it LGTM, thanks for the fix!

Copy link
Contributor

@renaynay renaynay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestNodeRPCPrefix looks broken, will take a look

@renaynay
Copy link
Contributor

This diff will fix the failing test, ws endpoint was taking the endpoint of the http server instead of ws

--- a/node/node_test.go
+++ b/node/node_test.go
@@ -558,7 +558,7 @@ func TestNodeRPCPrefix(t *testing.T) {
 func (test rpcPrefixTest) check(t *testing.T, node *Node) {
        t.Helper()
        httpBase := "http://" + node.http.listenAddr()
-       wsBase := "ws://" + node.http.listenAddr()
+       wsBase := "ws://" + node.ws.listenAddr()

        if node.WSEndpoint() != wsBase+test.wsPrefix {
                t.Errorf("Error: node has wrong WSEndpoint %q", node.WSEndpoint())

@fjl
Copy link
Contributor

fjl commented Feb 11, 2021

I think we put them on the same port on purpose when the port is zero. Is it a problem to have them on the same port in this case?

@holiman
Copy link
Contributor Author

holiman commented Feb 11, 2021

I think we put them on the same port on purpose when the port is zero. Is it a problem to have them on the same port in this case?

No, I thought it was more UX-friendly this way, but it's really just a "pick either option" kind of thing. Should I revert that?

@fjl
Copy link
Contributor

fjl commented Feb 11, 2021

I would prefer to keep them on the same port in this case.

@holiman holiman changed the title node: show websocket url + differentiate http/ws on both set to 0 node: show websocket url in logs Feb 12, 2021
@holiman
Copy link
Contributor Author

holiman commented Feb 12, 2021

Fixed

node/rpcstack.go Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Is there ever a case where neither ws nor rpc is allowed, but we still want to not exit here? I think the answer is no, but .... graphql..? cc @renaynay

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Graphql cannot be allowed if rpc is disabled

@fjl fjl merged commit b1835b3 into ethereum:master Feb 18, 2021
@fjl fjl added this to the 1.10.0 milestone Feb 18, 2021
JukLee0ira added a commit to JukLee0ira/XDPoSChain that referenced this pull request Jun 23, 2025
gzliudan pushed a commit to XinFinOrg/XDPoSChain that referenced this pull request Jun 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants