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

node: show websocket url in logs #22307

Merged
merged 1 commit into from
Feb 18, 2021
Merged

node: show websocket url in logs #22307

merged 1 commit into from
Feb 18, 2021

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

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

url := fmt.Sprintf("ws://%v", listener.Addr())
if h.wsConfig.prefix != "" {
url += h.wsConfig.prefix
}
h.log.Info("WebSocket enabled", "url", url)
}
// if server is websocket only, return after logging
if !h.rpcAllowed() {
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

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