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

Print the server's scheme when logging that it's reachable #585

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Joannis
Copy link
Member

@Joannis Joannis commented Oct 13, 2024

No description provided.

@Joannis Joannis requested a review from adam-fowler as a code owner October 13, 2024 17:21
@Joannis
Copy link
Member Author

Joannis commented Oct 13, 2024

Not sure if we can make this change - but I think it would be nice to log the scheme as described in #578. We can also assume HTTP (not HTTPS) here, but I'd prefer logging it correctly.

@Joannis
Copy link
Member Author

Joannis commented Oct 13, 2024

We can also consider detecting if the IP is :: or 0.0.0.0 so that we can print more accurate info

Copy link
Member

@adam-fowler adam-fowler left a comment

Choose a reason for hiding this comment

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

Seems a bit of overkill just to print http or https.
Could you not just check the first channel handler is NIOSSLServerHandler

@Joannis
Copy link
Member Author

Joannis commented Oct 14, 2024

Unfortunately no, as TransportServices does not have a handler, and these handlers are only applicable for child channels of the server

@Joannis Joannis force-pushed the jo/print-server-scheme branch from d30cdd4 to c2e3da3 Compare October 14, 2024 15:38
@Joannis Joannis requested a review from adam-fowler October 14, 2024 15:38
Copy link
Member

@adam-fowler adam-fowler left a comment

Choose a reason for hiding this comment

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

I'm not sure what the point of this is? Why do you want to print 127.0.0.1 when the bind address is 0.0.0.0

@Joannis
Copy link
Member Author

Joannis commented Oct 15, 2024

@adam-fowler my idea was that I want to (eventually) make it explicit how to reach it from various devices. Should've explained that in the PR.

So 0.0.0.0 should also render into your LAN IP and 127.0.0.1

@adam-fowler
Copy link
Member

@Joannis But bind address 0.0.0.0 is different from 127.0.0.1, ie one accepts connections from any address while the other accepts only connections from the localhost. We shouldn't be pretending they are the same.

Maybe if the bind address is set to 0.0.0.0 then we can add an additional log that says it is accepting connections from anywhere

@Joannis
Copy link
Member Author

Joannis commented Oct 15, 2024

I understand what 0.0.0.0 does, but it's not common knowledge. By adding logs about the available addresses to connect to, I'm hoping to address that knowledge gap so that people understand how this works. Or more importantly, how they can test their app.

@Joannis
Copy link
Member Author

Joannis commented Oct 15, 2024

An excellent example that I've had people ask time and time again:

How do I connect to my backend from an iOS app? 127.0.0.1 doesn't work.

@adam-fowler
Copy link
Member

You need an additional log item then, because the people who do know what is going on are going to be a little confused when there server logs says the server is listening on 127.0.0.1 but accepting connections from anywhere.

An excellent example that I've had people ask time and time again:

How do I connect to my backend from an iOS app? 127.0.0.1 doesn't work.

Also can we make it clear which situation we are trying to fix here, as your change doesn't fix this.

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.

2 participants