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

Also expose port for each connection #26

Closed
clue opened this issue Sep 19, 2015 · 8 comments · Fixed by #65
Closed

Also expose port for each connection #26

clue opened this issue Sep 19, 2015 · 8 comments · Fixed by #65

Comments

@clue
Copy link
Member

clue commented Sep 19, 2015

Currently, the Connection::getRemoteAddress() method only returns the IP address of the remote peer.

There's currently no way to access its TCP/IP port.

It's quite trivial to add a getRemotePort() method, but we should think twice about whether we should limit ourselves to TCP/IP here.

Other schemes (such as Unix or perhaps even raw sockets) do not have a concept of "ports" and may not necessarily build on top of IP.

As such, we should consider just returning the full remote peer address from the getRemoteAddress() method.

@clue
Copy link
Member Author

clue commented Apr 20, 2016

See also reactphp/reactphp#199

@SamMousa
Copy link

Alternatively it would be great to at least not remove it from the address.

Are there any plans to add this to the interface at some point?
To properly support proxying one would want to be able to override this function to specify the port / address of the other end of the tunnel... In such a scenario manually calling stream_get_name() does not suffice.

I'm not sure what the downside is of adding a getter for the port that returns null when there is no port (ie in case of a Unix socket). Alternatively we could create a TcpConnectionInterface that has a port.

Is any development / discussion on this topic being held elsewhere?

@clue
Copy link
Member Author

clue commented Jul 12, 2016

Are there any plans to add this to the interface at some point?

This ticket has received little interest so far. Care to change this an file a PR? :)

Alternatively it would be great to at least not remove it from the address.

I think this is what makes most sense, see also reactphp/reactphp#199 for more details.

@SamMousa
Copy link

@clue I've read reactphp/reactphp#199, the issue is that the longer we wait with making a choice the more libraries will break when we do decide to change it.

Just returning the result from stream_get_name is in my opinion not the best thing to do.
Of course it could be interesting to formalize addresses as well. (AddressInterface) but that might be overkill.

I'm definitely available to make a PR but let's first agree on the desired direction.
https://github.com/SAM-IT/react-socket

@clue
Copy link
Member Author

clue commented Jul 12, 2016

Just returning the result from stream_get_name is in my opinion not the best thing to do.

Care to elaborate what your opinion is based on?

Of course it could be interesting to formalize addresses as well. (AddressInterface) but that might be overkill.

I agree that this would be the cleanest and totally overkill solution :)

@SamMousa
Copy link

SamMousa commented Jul 12, 2016

Well the whole point of implementing a Connection is abstraction. Returning a string like that doesn't feel right to me.

@clue
Copy link
Member Author

clue commented Jul 12, 2016

For the most part, Connection is an abstraction of a duplex stream (plus a single accessor method for the remote address). Given the little interest in this ticket so far and looking at existing libraries building on top of this library, the remote address appears to used way less than the stream abstraction (from what I can tell).

Technically, the "port" is part of the "address" (for TCP/IP anyway).

We could introduce a second accessor method for the remote port and then rename the first method to make it obvious it does not include the port. However, this would be a leaky abstraction as we've now leaked the detail that we focus on TCP/IP connections and other protocols do not know the concept of a "port".

Alternatively, we could change the single accessor method to return the full address. The string format returned by the underlying stream_get_name() would probably be the simplest solution here. This is in line with how URIs are represented throughout React's components (or planned for the future as per reactphp/reactphp#199) and many other third-party libraries.

I agree that having an interface (kind of like PSR-7's UriInterface) sounds tempting here. If we want to implement this throughout React's components we'd have to add dependencies to a shared component. For consistency, we'd likely also type-hint against this and thus require users to create new instances before establishing connections to URIs or setting up listening addresses.

As such, I'm in favor of using a single method to access the URI and I'm not really convinced an additional interface is actually worth the hassle. I'm curious what others think about this?

@SamMousa
Copy link

That sounds ok to me, and I agree that doing a true abstraction requires too much overhead and a leaky abstraction is not what we want; so that leaves getRemoteAddress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants